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

Update docs with reactStrictMode in render #1479

Merged
merged 3 commits into from
Apr 2, 2025

Conversation

bernardobelchior
Copy link
Contributor

Update docs with reactStrictMode in render, introduced in testing-library/react-testing-library#1390.

Copy link

netlify bot commented Apr 2, 2025

Deploy Preview for testing-library ready!

Name Link
🔨 Latest commit af9fb45
🔍 Latest deploy log https://app.netlify.com/sites/testing-library/deploys/67ed69591cf0d600082b3b7c
😎 Deploy Preview https://deploy-preview-1479--testing-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -31,7 +32,8 @@ as these methods:
- [`initialProps`](#initialprops)
- [`onCaughtError`](#oncaughterror)
- [`onRecoverableError`](#onrecoverableerror)
- [`wrapper`](#wrapper-1)
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 link was wrong

@bernardobelchior bernardobelchior marked this pull request as ready for review April 2, 2025 15:13
@@ -161,6 +163,11 @@ utility functions to create custom queries.
Custom queries can also be added globally by following the
[custom render guide](setup.mdx#custom-render).

### `render` Options `reactStrictMode`
Copy link
Member

@eps1lon eps1lon Apr 2, 2025

Choose a reason for hiding this comment

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

Suggested change
### `render` Options `reactStrictMode`
### `reactStrictMode`

Isn't this redundant? Also don't forget to update the anchor if you accept the change.

Copy link
Contributor Author

@bernardobelchior bernardobelchior Apr 2, 2025

Choose a reason for hiding this comment

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

Naming it only reactStrictMode would conflict with reactStrictMode from configure because they would share the same hash. If you prefer another name, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

It's nested below "render Options" already so there shouldn't be a conflict. Or do you mean the anchors conflict? Can we decouple the anchor ID from the heading name?

This just seems noisy at the moment:

- render Options
   - container
   - baseElement
   - [...]
   - render Options reactStrictMode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean the anchors conflict?

Exactly.

Can we decouple the anchor ID from the heading name?

It seems auto-generated. How would you suggest we do that?

This just seems noisy at the moment:

Yeah, I agree, but I was following the same pattern as renderHook Options wrapper.

Happy to change, but I'm not sure if there's an easy solution.

Copy link
Member

Choose a reason for hiding this comment

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

Then let's keep it for now. Could even argue the repeated context is good on nested list with lots of siblings since it reduces cognitive overhead.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

I'd also cross reference this option from configure#reactStrictMode to help discoverability.

@bernardobelchior
Copy link
Contributor Author

I'd also cross reference this option from configure#reactStrictMode to help discoverability.

Done. Let me know if wording is fine or if there's any suggestion for improvement.

@eps1lon eps1lon merged commit d26e8ca into testing-library:main Apr 2, 2025
4 checks passed
@bernardobelchior bernardobelchior deleted the patch-2 branch April 3, 2025 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants