Skip to content
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

Use Admonitions on website #12495 #12951

Closed
wants to merge 21 commits into from
Closed

Conversation

paulreece
Copy link
Contributor

Summary

I have changed all of the improperly formatted "note" sections to now use admonitions instead.

Test plan

The code is just markdown edits so I'm sure it will pass all tests.

@facebook-github-bot
Copy link
Contributor

Hi @paulreece!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@mrazauskas
Copy link
Contributor

Thanks! Could you split the PR, please?

That is rather large change and it is hard to review everything in one go.

It would be perfect to keep one file (or two smaller) per PR.

@paulreece
Copy link
Contributor Author

@mrazauskas So you want me to split out each md file that I edited? Like 20 PRs? There's only a couple of changes per file, just changing the old note style to the admonition style.

@mrazauskas
Copy link
Contributor

Right, but I would like to be able to look through the whole source of each file. Just to make sure that there will be no need to come back to these files.

I did review similar PR previously and to look through one or two file was fine. More is too much.

Also note that in the issue it was agreed to keep the PRs small.

Comment on lines 130 to 134
:::

:::note

Note: this method was previously called `autoMockOn`. When using `babel-jest`, calls to `enableAutomock` will automatically be hoisted to the top of the code block. Use `autoMockOn` if you want to explicitly avoid this behavior.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you look through this file again? Seems like you are only adding new lines, which are duplicating information which already exists in the document.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the new lines and duplicated messages. Please let me know if there's anything else needed to change.

@@ -715,6 +754,56 @@ Returns the `jest` object for chaining.

Restores all mocks back to their original value. Equivalent to calling [`.mockRestore()`](MockFunctionAPI.md#mockfnmockrestore) on every mocked function. Beware that `jest.restoreAllMocks()` only works when the mock was created with `jest.spyOn`; other mocks will require you to manually restore them.


### `jest.mocked<T>(item: T, deep = false)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jest.mocked is already documented. Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a variation on jest.mocked and was preexisting from the Docs I forked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Jest 29 this method has different API. Documentation got reworked accordingly. I don’t see why outdated documentation should be added, if the goal of your PR is to add missing admonitions. That it definitely out of scope. If there are any issues with current documentation of jest.mocked, please open a separate PR.

@@ -910,8 +999,24 @@ This function is not available when using legacy fake timers implementation.

### `jest.setTimeout(timeout)`


Set the default timeout interval (in milliseconds) for all tests and before/after hooks in the test file. This only affects the test file from which this function is called.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you adding this text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why it is showing that I added it but I double checked in the main app and this is preexisting text.

@@ -5,6 +5,16 @@ title: ECMAScript Modules

:::caution


:::note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed now.

Comment on lines 476 to 480
:::info

:::note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why note is better than info?

Comment on lines 120 to 119
:::tip
:::note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why note is better than tip?

Comment on lines 6 to 12
:::caution

Note that due to its experimental nature there are many bugs and missing features in Jest's implementation, both known and unknown. You should check out the [tracking issue](https://github.com/facebook/jest/issues/9430) and the [label](https://github.com/facebook/jest/labels/ES%20Modules) on the issue tracker for the latest status.

:::

:::note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already a caution block, why to split it?

docs/CLI.md Outdated

:::note

Note that if configuration files are found in the specified paths, _all_ projects specified within those configuration files will be run.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can skip the "Note that" starts to all these sentences

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These have been removed from all .md files that have Note admonitions.

Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again I would like to suggest splitting CLI.md to a separate PR. If versioned docs would be corrected accordingly (as an example see #13284), that would become a solid PR.

docs/CLI.md Outdated Show resolved Hide resolved
docs/CLI.md Outdated Show resolved Hide resolved
docs/CLI.md Outdated
@@ -180,8 +180,11 @@ Alias: `--collectCoverage`. Indicates that test coverage information should be c

Indicates which provider should be used to instrument code for coverage. Allowed values are `babel` (default) or `v8`.

Note that using `v8` is considered experimental. This uses V8's builtin code coverage rather than one based on Babel. It is not as well tested, and it has also improved in the last few releases of Node. Using the latest versions of node (v14 at the time of this writing) will yield better results.
:::note
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be already removed in the latest version of the docs and stay only in older ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it.

docs/CLI.md Outdated Show resolved Hide resolved
paulreece and others added 4 commits September 21, 2022 20:56
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@paulreece
Copy link
Contributor Author

@SimenB @mrazauskas I split the files into separate PRs. Here ya go:

#13293
#13294
#13295
#13296
#13297
#13298
#13299
#13300
#13301
#13302
#13303
#13304
#13305
#13306
#13307
#13308
#13309
#13311

@mrazauskas
Copy link
Contributor

Perfect! Thank you so much. This is very helpful.

I have left reviews in all the PR. Here is a couple of more general notes:

@paulreece
Copy link
Contributor Author

@mrazauskas To Clarify were you all wanting admonitions in each of the files in each sub directory of website/versioned_docs?

@mrazauskas
Copy link
Contributor

Thanks a lot for the effort. Just left comments with more details in each PR. Yesterday I was in hurry to look through everything. Hope now all is clarified.

Yes, I had in mind all sub directories of website/versioned_docs. That should make six files per PR with the similar changes in each file. Keep in mind that some notes might not exist in older docs, because some features were added later. If notes are not there, no need to add them, of course.

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

Thank you both so much for taking. the time to split this up and review it! ❤️

I've been neck deep in native app development the last couple of weeks (and will for the next few as well) and haven't had the time to dedicate to going through them. So I very much appreciate the bite sized, and reviewed, PRs 👍


I think we can close this one, tho, as the others should be merged.

@SimenB SimenB closed this Sep 28, 2022
@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

Note that I'll be rolling a 29.1 version, so the remaining two (#13295 and #13310) should get an additional version before landing 🙂

@paulreece
Copy link
Contributor Author

@SimenB or @mrazauskas Would it be possible for you to ping me on here when you do roll out the 29.1 versions so I can then update #13295 and #13310 accordingly? Thanks so much to @mrazauskas for taking the time to help me get these revisions ready for production!

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

29.1 has been out for about 10 hours 🙂

https://github.com/facebook/jest/releases/tag/v29.1.0

@paulreece
Copy link
Contributor Author

Awesome thanks!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants