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 Restore gridfield state from get vars (POC) #10331

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented May 22, 2022

The GridFieldDetailForm's item edit form adds the current gridfield state to the URL of the CMS breadcrumbs back button (not the browser back button).
This PR pulls that state back into the GridField, so that when a user clicks back they retain the pagination and filter options they had set.

Parent Issue

Why this is draft

The GridField is a complicated beast, and apparently there are a few issues related to this one which ideally should be addressed in tandem. Also, trying to resolve this is taking a lot longer than I expected, so I'm parking this for now.
Note that this PR is not complete - for this functionality to be reliable we would need to make the following additional changes:

  • Add the state to the GridFieldViewButton's URL
  • Check why sometimes the GridFieldEditButton doesn't include the state and decide if it should do so
  • Add javascript to admin in GridField.js in the onmatch event to make the search filter visible if the state indicates that the field has been filtered
  • Consider using window.history.replaceState() in admin javascript to add the gridfield state to the current history when the state changes (i.e. when paginating or filtering a gridfield)

Related PR

@sabina-talipova
Copy link
Contributor

Hi @GuySartorelli ,

Are you planning to continue work on this task? Or do you want me to finish it?
Thank you.

@GuySartorelli
Copy link
Member Author

@sabina-talipova I'm not going to take this any further at this stage. I think we need to take a step back and look at how the state for gridfield is being managed more generally - as mentioned in my last comment there are quite a few issues with gridfield state which point to a deeper problem than this PR is equipped to resolve.

@GuySartorelli
Copy link
Member Author

This doesn't seem to work at all for relations in a gridfield on a page. e.g. try adding some relations to the "has many" or "many many" tabs in the "GridFieldTestPage" created by framework test.
Seems to work okay for gridfields in a model admin, but doesn't have browser back/forward support - I assume you're holding off on that part until we have some UX advice on how that should behave?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Jul 18, 2022

There's no code in here or in the companion admin PR to actually set the gridfield state to the URL when you perform some action (i.e. paginate or filter) - so the state is only appended to the URL for the GridFieldEditButton specifically - and that's only there to support the breadcrumb back button.
This means that GridFieldViewButton for example has no state attached to it, which means breadcrumb back buttons for view-only forms won't return you to the state you were in. It also means I can't copy the URL to a new tab and keep my state in that new tab.

The following ACs from the issue are therefore not met:

  • When you perform an action in the gridfield, it updates the URL.
    Currently the state is not in the URL except when you click on a GridFieldEditButton and then on the back button in the breadcrumbs
  • The browser history is hijacked so that the back/next button can be use to navigate your gridfield state.
    I assume you're waiting for the UX advice on this, so that is fine for now

Interestingly there's no AC for the breadcrumb back button, but there probably should be.

@sabina-talipova
Copy link
Contributor

There's no code in here or in the companion admin PR to actually set the gridfield state to the URL when you perform some action (i.e. paginate or filter) - so the state is only appended to the URL for the GridFieldEditButton specifically - and that's only there to support the breadcrumb back button. This means that GridFieldViewButton for example has no state attached to it, which means breadcrumb back buttons for view-only forms won't return you to the state you were in. It also means I can't copy the URL to a new tab and keep my state in that new tab.

The following ACs from the issue are therefore not met:

  • When you perform an action in the gridfield, it updates the URL.
    Currently the state is not in the URL except when you click on a GridFieldEditButton and then on the back button in the breadcrumbs
  • The browser history is hijacked so that the back/next button can be use to navigate your gridfield state.
    I assume you're waiting for the UX advice on this, so that is fine for now

Interestingly there's no AC for the breadcrumb back button, but there probably should be.

To keep State in URL after user click on search or sort fields, I had to make changes in GridField.js. All this changes in other PR: silverstripe/silverstripe-admin#1322

@sabina-talipova sabina-talipova force-pushed the pulls/4/gridfield-keep-state branch from f5197d5 to 71bf390 Compare July 19, 2022 03:05
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldEditButton.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldStateManager.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4/gridfield-keep-state branch 5 times, most recently from 93ff035 to 8deecfa Compare July 20, 2022 21:38
@sabina-talipova sabina-talipova force-pushed the pulls/4/gridfield-keep-state branch 3 times, most recently from 083f603 to 815db06 Compare July 24, 2022 20:52
@sabina-talipova sabina-talipova force-pushed the pulls/4/gridfield-keep-state branch from 815db06 to f74b472 Compare July 25, 2022 02:01
@sabina-talipova sabina-talipova marked this pull request as ready for review July 25, 2022 21:05
Copy link
Member Author

@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.

In GridFieldDetailForm_ItemRequest::getBackLink() change the return statement to this:

return $this->gridField->addOtherGridfieldStateToURL($backlink);

That way we don't have to do that in LeftAndMain or CMSMain etc.

src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridField.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldEditButton.php Outdated Show resolved Hide resolved
tests/php/Forms/GridField/GridFieldTest.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

There's a failing unit test - other than that it looks good.

@sabina-talipova sabina-talipova force-pushed the pulls/4/gridfield-keep-state branch from 291ce3e to 7a9bc7f Compare July 27, 2022 22:15
@sabina-talipova
Copy link
Contributor

@GuySartorelli, actions failed on unrelated to my changes test cases. Should I fix them?

@GuySartorelli
Copy link
Member Author

No, you can ignore that for this PR. It should be fixed separately. It looks like it was caused by a bad merge up from CI PRs, so Steve or I will look at it as part of the CI migration work

@GuySartorelli
Copy link
Member Author

Failed unit test is unrelated.

@GuySartorelli
Copy link
Member Author

I can't click "approve" since I originally opened this PR - so consider this message to be approval.

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