-
Notifications
You must be signed in to change notification settings - Fork 608
Updated DataTable for alternate key #5907
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
base: main
Are you sure you want to change the base?
Updated DataTable for alternate key #5907
Conversation
🦋 Changeset detectedLatest commit: c9eb5bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
Hey there! Thanks for taking the time to make a PR for this 🙏
Taking a look real quick, I think we'd either want to add a top-level getRowId
function (similar to tanstack table) or would want to extend UniqueRow
to support a function to get the id.
We would also want to make sure this gets passed along to useTable
so that row.id
matches what is provided.
Let me know what you think! Curious to hear your thoughts 👀
I totally agree—adding a top-level getRowId function or extending UniqueRow to support a custom ID getter makes a lot of sense. I’ll update the PR to reflect that and make sure it flows through to useTable so row.id aligns properly. |
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 looking great, thanks so much for all the changes 🙏 Would you mind adding a test scenario for this to make sure we have it captured in our test coverage?
Since the key prop in React is used internally for rendering and does not appear in the DOM, would it be appropriate to verify the correct number of rendered elements as a means of testing, or would you recommend a more effective approach to validate this behavior? |
You could test not DataTable, but the useTable function itself and validate that it overrides the row.id with the result of getRowId function. Similarly add test cases for when no getRowId function is supplied |
Co-authored-by: Marie Lucca <[email protected]>
Thanks for all the work on this @bibektamang7 🙏 ! |
Head branch was pushed to by a user without write access
Thank you so much! Really appreciate your thoughtful review and support 🙏 Excited to keep contributing! |
Closes #5898
New
Changed
Rollout strategy
Testing & Reviewing
Merge checklist