-
Notifications
You must be signed in to change notification settings - Fork 294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Minor docs updates for the migration guides #1116
Minor docs updates for the migration guides #1116
Conversation
Pull Request Test Coverage Report for Build 7978235888Details
💛 - Coveralls |
Added a PR. #1116 After looking again, I'm not sure the PR template needs any changes. It would be nice if there was a way for VSCode to dump the effective settings for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, looks good, just one minor adjustment and we should be good to go.
README.md
Outdated
@@ -444,7 +444,7 @@ The extension also features built-in conflict detection and quick fixes to assis | |||
"clearOnRun": "terminal" | |||
} | |||
``` | |||
|
|||
**Migration Guide** | |||
<a id="outputconfig-migration"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put the tag above the title
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting a simple swap like this:
<a id="outputconfig-migration"></a>
**Migration Guide**
No need for additional alert
c399e71
to
0fa3e69
Compare
0fa3e69
to
e116a40
Compare
- Finally, remove any deprecated settings. | ||
|
||
By following these guidelines, you should be able to smoothly transition to using `"jest.outputConfig"`. | ||
> [!NOTE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need the [!NOTE]
here. Sorry if my earlier message didn't come across clearly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty minor, never mind. I can adjust it later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's what I thought you meant at first. I was on the fence about whether the note makes it better or worse so I left it up to you.
The length of the README is getting unwieldy. If it sounds good to you, I might start sending PRs to break it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @pmcelhaney!
- Finally, remove any deprecated settings. | ||
|
||
By following these guidelines, you should be able to smoothly transition to using `"jest.outputConfig"`. | ||
> [!NOTE] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is pretty minor, never mind. I can adjust it later if needed.
Yes, the README is getting pretty hard to go through. I am interested in hearing your idea of improving it... |
The main change is to updating the migration for
autoRevealOutput
to point tooutputConfig
instead ofrunMode
.I also updated the formatting to use alerts syntax. If you don't like it, I'm happy to update the PR.