-
Notifications
You must be signed in to change notification settings - Fork 843
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
[EuiPagination] Extract PaginationButton
creation
#5048
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5048/ |
CI errors will be resolved when the docs change is reverted |
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.
QA'd the fix and it looks great! Super impressed by the clean and elegant solution to the problem also, huge kudos. Feel free to revert the resizable documentation example - I'll approve after that.
I mostly have small comments/questions that are by no means blockers, but would def appreciate your thoughts!
Preview documentation changes for this PR: https://eui.elastic.co/pr_5048/ |
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.
Changes look great - thanks a million Greg!
I think resizable_container_basic.js
changes still need to be reverted, but other than that looks GTG!
This reverts commit 6a99e4b.
Preview documentation changes for this PR: https://eui.elastic.co/pr_5048/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5048/ |
Summary
Fixes #5046, in which EuiPagination failed to update state in EuiResizableContainer. The problem was not with EuiResizableContainer specifically, but could occur anytime a pagination change resulted in a parent rerender. Because the internal
PaginationButton
component was created inside another component, it was recreated with old state on every rerender.The solution here is to extract
PaginationButton
creation, eliminating rerender recreation.See the to-be-reverted change in the first EuiResizableContainer example for verification.
Checklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Props have proper autodocs and playground toggles- [ ] Added documentation- [ ] Added or updated jest testsrevert me
docs commit