-
Notifications
You must be signed in to change notification settings - Fork 96
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 Clear filter from url state #1580
FIX Clear filter from url state #1580
Conversation
513f2fc
to
88357e2
Compare
88357e2
to
d407f02
Compare
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.
Steve, it seems to me that now it doesn’t work quite the way it worked before. Because when search for smth and we go to the next level, for example, we click on one of the list elements and there we go to one of the nested lists and when we go back, we lose all the previous settings (filters, sorting and pagination).
Screen.Recording.2023-09-13.at.1.56.49.PM.mov
@@ -23,3 +28,15 @@ Feature: Search in GridField | |||
And I click "Walmart" in the "#Form_EditForm" element | |||
Then I should see "Walmart" | |||
And I should see "Walmart" in the ".breadcrumbs-wrapper" element | |||
|
|||
@sboyd |
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.
@sboyd |
d407f02
to
594db46
Compare
I think this is an existing behaviour, I can replicated that on 4.13 without this pull-request The gridState is lost when you view the individual employee "Troy" |
client/src/legacy/GridField.js
Outdated
// aggressive in removing their state. This limitation is because I couldn't find an | ||
// easy way to extract the gridfield number from the gridfield | ||
if (key.match(new RegExp(`^gridState\\-${gridFieldName}\\-[0-9]$`))) { | ||
console.log('Removing ' + key); |
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.
console.log('Removing ' + key); |
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.
Removed
There are also some failed tests. Please, have a look. |
fa1b4ae
to
0d7985b
Compare
0d7985b
to
9ba03f3
Compare
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.
Just small changes.
b9c7088
to
7ed648b
Compare
7ed648b
to
0b7d69b
Compare
0b7d69b
to
f9a151f
Compare
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.
Looks good to me. Works nice in local environment.
Issue #1573
Requires the following PRs merged first:
Installer run with all 3x PRs together:
Have run
yarn upgrade webpack
because js build was randomly failing - relates to #1582