-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(DataTable): guard npe when cells being sorted do not exist #9714
fix(DataTable): guard npe when cells being sorted do not exist #9714
Conversation
DCO Assistant Lite bot All contributors have signed the DCO. |
I have read the DCO document and I hereby sign the DCO. |
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: d480901 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/614b482278b10900081d9e22 😎 Browse the preview: https://deploy-preview-9714--carbon-react-next.netlify.app |
I have read the DCO document and I hereby sign the DCO. |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: 2be4546 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/614a4616ff46390007013e72 😎 Browse the preview: https://deploy-preview-9714--carbon-elements.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: 2be4546 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/614a4616e0721f00089e4131 😎 Browse the preview: https://deploy-preview-9714--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: d480901 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/614b482212300400074dde07 😎 Browse the preview: https://deploy-preview-9714--carbon-components-react.netlify.app |
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: d480901 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/614b4822bd54fe0007119ec2 😎 Browse the preview: https://deploy-preview-9714--carbon-elements.netlify.app |
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 taking a look at this! Previously I've recommended the use of a key
or the sortBy
function to correct the sort state when a column is removed from the table.
#9512 (comment)
This PR definitely makes sense - the use of a key
is no longer necessary, and there is still the option to use the sortBy
function if reverting to unsorted is not desired.
It would be nice if a new test case was added to the sorting tests to prevent any regression of this functionality, but won't hold up a merge
describe('sorting state', () => { |
@tay1orjones - thanks for your review! Unit tests added. |
Thanks for the contribution! |
Amazing @holmansze - thanks! 🎉 |
Thanks @tay1orjones and @jnm2377 ! |
Closes #9713
DataTable throws an NPE when the cells being sorted do not exist.
We have a table settings feature for DataTable where users can show or hide table columns. When the column is hidden, it is removed from
headers
prop to DataTable. If this column happens to be used for sorting, an NPE is thrown.The fix is simply an NPE check in the sorting code. When the cells being sorted do not exist, the sort compare function returns 0 (equal), and the table renders in the original (unsorted) order.
Changelog
New
Changed
Removed
Testing / Reviewing
New code tested by removing the column being sorted from the
headers
prop. Worked as expected - table renders properly without sorting. When the column is added back, the table becomes sorted.