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] Push browser history on pagination in react listviews #9624
Conversation
It might be worth pushing a change to trigger a rebuild - looks like the JS build failed due to a flake (Javascript OOM) |
c010c34
to
01ac7ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #9624 +/- ##
==========================================
- Coverage 70.52% 70.52% -0.01%
==========================================
Files 574 574
Lines 30152 30160 +8
Branches 3060 3072 +12
==========================================
+ Hits 21265 21270 +5
- Misses 8776 8778 +2
- Partials 111 112 +1
Continue to review full report at Codecov.
|
Triggered a rerun for the failed tests |
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.
LGTM. It's a working solution though a little repetitive. It looks like there's some issues syncing state, since we're storing it in various places (query params, internal filters, actual state that triggers data fetching). Part of the reason for this fragmentation is due to changing requirements (holding state until submit button is pressed, storing state in queryparams, etc.). Once the dust settles on these listview redesigns we should come back and try to consolidate state as much as possible. eg, we can create a hook called useStateWithQueryParam which would abstract away the queryParams state for us.
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.
LGTM!
CATEGORY
Choose one
SUMMARY
Before:
After:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
https://www.loom.com/share/2aa499227bb044fc9728a6910f01f650
AFTER:
https://www.loom.com/share/f049bbb13cc848d2ad14bdce11bf437d
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@rusackas @nytai