-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add hook that returns disabled value in RowContext #11759
Conversation
Co-authored-by: Chloe Rice <[email protected]>
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 looks good to me! Though, would it be better to have a single hook for getting RowContext
data, then adding more context data as needed?
const {disabled, hovered, selected, ...etc } = useIndexTableRow()
Then we could clean up the single-property hooks like useRowHovered
and useRowSelected
.
Absolutely down to combine all consumption of The choice to keep that in its own context provider was to reduce the potential performance hit of leaf nodes rerendering due to hover state changing in context, when other context values that are less frequently updated are being used. (Since context updates only re-renders for children that use that context, not all children of the context provider) Happy to combine |
0c3af34
to
f2c62d0
Compare
Localization quality issues found The following issues may affect the quality of localized translations if they are not addressed:
Please look out for other instances of this issue in your PR and fix them as well if possible. Questions about these messages? Hop in the #help-localization Slack channel. |
Hi! We noticed there hasn’t been activity on this PR in a while. After 30 days, it will close automatically. If it’s still relevant, or you have updates, comment and let us know. And don’t worry, you can always re-open later if needed. |
WHY are these changes introduced?
We don't yet expose the
disabled
value for children of theRowContext.Provider
to consume.This is something we discovered we need in the following web PR
See problem outlined in the following comment.
We already expose the
hover
andselected
context values via the useRowHovered and useRowSelected hooks respectively. This aligns with that convention.WHAT is this pull request doing?
Following convention established in the
useRowHovered
anduseRowSelected
hooks to return as small a slice of state as possible, we expose auseRowDisabled
hook so that context children can access thedisabled
state of the parent Row. (Needed for https://github.com/Shopify/web/pull/121521)How to 🎩
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
🎩 checklist
README.md
with documentation changes