-
Notifications
You must be signed in to change notification settings - Fork 2.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 issue in GroupedList where paging new data into existing groups did not re-render #15335
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3a0e570:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changesUnable to find bundle size details for Baseline commit: d5b0802 Possible causes
Recommendations
|
d77b0b2
to
3a0e570
Compare
🎉 Handy links: |
…did not trigger re-render (#15628) <!-- !!!!!!! IMPORTANT !!!!!!! Due to work we're currently doing to prepare master branch for our version 8 beta release, please hold-off submitting the PR until around October 12 if it's not urgent. If it is urgent, please submit the PR targeting the 7.0 branch. This change does not apply to react-northstar contributors. See #15222 for more details. Sorry for the inconvenience and short notice. --> #### Pull request checklist - [ ] Addresses an existing issue: Fixes #0000 - [x] Include a change request file using `$ yarn change` #### Description of changes _Cherry-pick of #15335._ _Original PR description:_ Reworked the logic in the new `getDerivedStateFromProps` function in `GroupedList` so it properly re-renders the `List` if any of the dependent props change. This fixes an issue where updating only the `items` would not actually trigger a re-render because they were not explicitly passed as props to the child `List` components. #### Focus areas to test Added a new unit test to validate updating the `items` in-place with the same groupings, using some placeholders to simulate paging.
…did not trigger re-render (microsoft#15628) <!-- !!!!!!! IMPORTANT !!!!!!! Due to work we're currently doing to prepare master branch for our version 8 beta release, please hold-off submitting the PR until around October 12 if it's not urgent. If it is urgent, please submit the PR targeting the 7.0 branch. This change does not apply to react-northstar contributors. See microsoft#15222 for more details. Sorry for the inconvenience and short notice. --> #### Pull request checklist - [ ] Addresses an existing issue: Fixes #0000 - [x] Include a change request file using `$ yarn change` #### Description of changes _Cherry-pick of microsoft#15335._ _Original PR description:_ Reworked the logic in the new `getDerivedStateFromProps` function in `GroupedList` so it properly re-renders the `List` if any of the dependent props change. This fixes an issue where updating only the `items` would not actually trigger a re-render because they were not explicitly passed as props to the child `List` components. #### Focus areas to test Added a new unit test to validate updating the `items` in-place with the same groupings, using some placeholders to simulate paging.
Description of changes
Reworked the logic in the new
getDerivedStateFromProps
function inGroupedList
so it properly re-renders theList
if any of the dependent props change. This fixes an issue where updating only theitems
would not actually trigger a re-render because they were not explicitly passed as props to the childList
components.Focus areas to test
Added a new unit test to validate updating the
items
in-place with the same groupings, using some placeholders to simulate paging.