Skip to content
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

ENH Keep State and show Search field if any keywords are presented #1326

Conversation

sabina-talipova
Copy link
Contributor

Description

  • Add a check for the presence of a filter in the state that is stored in the URL. If exist, keep Search field open.
  • Keep state in URL when new filter was applied or GridField was sorted

Parent issue

@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jul 19, 2022

Open instead of #1322

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR should target the 1 branch - it doesn't make any sense to release this separately from silverstripe/silverstripe-framework#10331

The UX advice we received indicated the following behaviour for the history state:

  • If you navigate to a ModelAdmin or page that has a gridfield and then immediately click 'back' in your browser, you should go back to the previous page you were on.
  • If you navigate to a ModelAdmin or page that has a gridfield and then change the state of that gridfield (either one time or many times e.g. sort, filter, paginate a couple of times), when you click 'back' in your browser you should go back to the initial default state of the gridfield. Pressing 'back' again will then take you back to the page you were previously on.

The first of those is met, but the second is not.

code/LeftAndMain.php Outdated Show resolved Hide resolved
code/SecurityAdmin.php Outdated Show resolved Hide resolved
client/src/legacy/GridField.js Show resolved Hide resolved
client/src/legacy/GridField.js Outdated Show resolved Hide resolved
client/src/legacy/GridField.js Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova changed the base branch from 1.11 to 1 July 26, 2022 00:46
@sabina-talipova sabina-talipova changed the base branch from 1 to 1.11 July 26, 2022 00:47
@sabina-talipova sabina-talipova changed the base branch from 1.11 to 1 July 26, 2022 00:47
tests/behat/features/gridfield-search.feature Outdated Show resolved Hide resolved
tests/behat/features/gridfield-search.feature Outdated Show resolved Hide resolved
code/SecurityAdmin.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/1.11/gridfield-keep-state branch 2 times, most recently from b4f5f6f to 3aae27e Compare July 26, 2022 20:58
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add behat tests for navigating back/forward in the browser.
This can be done with When I move backward one page and When I move forward one page

Also, have you addressed my earlier comment about the UX advice not being implemented correctly?

client/src/legacy/GridField.js Outdated Show resolved Hide resolved
tests/behat/features/gridfield-search.feature Outdated Show resolved Hide resolved
client/src/legacy/GridField.js Show resolved Hide resolved
@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jul 27, 2022

#1326 (review)
Could I clarify behavior for "Forward" button in browser? What is your expectations?

@GuySartorelli
Copy link
Member

Could I clarify behavior for "Forward" button in browser? What is your expectations?

If you click the "back" button in the browser, and then click the "forward" button in the browser, you should be looking at the exact same page with the same state as you had before you clicked "back".

@sabina-talipova
Copy link
Contributor Author

sabina-talipova commented Jul 27, 2022

If you click the "back" button in the browser, and then click the "forward" button in the browser, you should be looking at the exact same page with the same state as you had before you clicked "back".Z

Sorry, Guy, but I completely disagree with this approach.
We don't have any navigation tools for GridField list of items.
Just imagine, user sorted and filtered list and now he is on some page of results. Then he accidentally or somehow clicked "Back" button and lost his results. Then he decided to use "Forward" button to come back to his previous state, but he stay on the same page.
If you test Google, how it works. It keeps each step in the history to provide users with ability to navigate back and forward.
Probably, better solution to provide "Clear" button for sorting and filtering.

@sabina-talipova sabina-talipova force-pushed the pulls/1.11/gridfield-keep-state branch 2 times, most recently from 6ca493b to e72ed94 Compare July 28, 2022 01:28
@GuySartorelli
Copy link
Member

Just putting this here for the sake of anyone coming across this PR in the future:
We had a follow-up UX session and the following advice came out of it (note that "page" here should not be interpreted to mean pagination):

  • Clicking back in the browser should take you to the previous page you were on
  • Clicking forward in the browser should take you forward one page

i.e:

  1. I am at /admin/pages
  2. I navigated to some model admin which has a gridfield
  3. I paginate through, sort, or filter the gridfield
  4. I click back - this takes me back to /admin/pages
  5. I click forward - this takes me to the model admin gridfield with whatever sort/pagination/filtering I had applied before step 4.

@sabina-talipova sabina-talipova force-pushed the pulls/1.11/gridfield-keep-state branch from e72ed94 to eb61b0d Compare July 28, 2022 01:59
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One very minor change to make.

client/src/legacy/GridField.js Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/1.11/gridfield-keep-state branch from eb61b0d to 55beb10 Compare July 28, 2022 03:45
Comment on lines 211 to 212
const historyState = $.extend({}, { page: window.location.pathname + searchParam }, this.getState());
history.replaceState(historyState, '', window.location.pathname + searchParam);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const historyState = $.extend({}, { page: window.location.pathname + searchParam }, this.getState());
history.replaceState(historyState, '', window.location.pathname + searchParam);
window.ss.router.replace(window.location.pathname + searchParam, undefined, undefined, false);

Turns out we have to use the special custom page router that Silverstripe admin uses... see https://github.com/visionmedia/page.js/blob/master/page.js#L678 for what this is actually doing.

Comment on lines 36 to 40

$(window).on('popstate', function (e) {
if (e.originalEvent.state === null) { return; }
location.reload();
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what's causing the behat failures.

Suggested change
$(window).on('popstate', function (e) {
if (e.originalEvent.state === null) { return; }
location.reload();
});

@sabina-talipova sabina-talipova force-pushed the pulls/1.11/gridfield-keep-state branch from 55beb10 to b86259d Compare July 28, 2022 22:43
@GuySartorelli GuySartorelli merged commit c88cd5b into silverstripe:1 Jul 28, 2022
@GuySartorelli GuySartorelli deleted the pulls/1.11/gridfield-keep-state branch July 28, 2022 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants