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

BUG Do not override grid state with search parameters #1103

Conversation

maxime-rainville
Copy link
Contributor

There's a bit of logic hat overrides the girdstate when you click the detail button with your search parameter, which might not reflect your current gridstate.

This video illustrate the original problem and the end results when the patch is applied.

https://youtu.be/Jw-zdpkXe-Q

Parent issue

@sminnee
Copy link
Member

sminnee commented Aug 28, 2020

I think that we should get better about embedding grid state changes into the URL using the history js api

Benefits

  • back button from a detail form goes back to the correct state
  • copy/paste grid field URL goes to the state
  • saving a multi-page editable gridfield returns to the right page

The main worry I would have is multiple grid fields on a single form, to check that the grid state is linked to the right grid.

I raise this here because I'm not sure if such work would conflict with this PR?

@maxime-rainville
Copy link
Contributor Author

An alternative approach could be to make sure that any grid state change are immediately reflected in the URL. Even if we go with this kind of approach, I don't think we should directly read the GET parameters from the URL. We should have some sort of abstraction that can tell us what the current grid state is.

When you first access the a given URL, that abstraction would be hydrated with the grid state provided in the URL. Then has the grid sate changes, the abstraction would update the URL using the History JS API.

@sminnee
Copy link
Member

sminnee commented Sep 8, 2020

An alternative approach could be to make sure that any grid state change are immediately reflected in the URL

Sorry that's what I meant by "embedding grid state changes into the URL using the history js api"

I believe that with that in place, the server-side render of GridField should be able to do what's needed without special post-render treatment being needed in the JavaScript. At the very least, I would start by setting that theory using some handcrafted URLs.

I agree that the actual manipulation of the gridstate on the client side should be captured in a shared helper of some kind.

@bergice bergice linked an issue Sep 14, 2020 that may be closed by this pull request
5 tasks
Copy link
Contributor

@bergice bergice left a comment

Choose a reason for hiding this comment

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

I couldn't get this working.

When going back to the previous URL from the detail view, nothing is filtered and the query params are still empty.
When clicking the back button, the query params are present, but it's not filtering anything.

@maxime-rainville
Copy link
Contributor Author

When going back to the previous URL from the detail view, nothing is filtered and the query params are still empty.
When clicking the back button, the query params are present, but it's not filtering anything.

That's not exactly what this thing was trying to fix (although yes, we should fix that).

The bug I was trying to fix is this.

  • Access a ModalAdmin
  • Sort by one column.
  • Click on one entry. You'll be taken to the EditDetail form. Notice that when using "better-button" to navigate the result, the sorting order is honoured.
  • Return to the list view.
  • Sort by a different column.
  • Click on one entry. Try navigating results using "better-button".

Expected results: The next result returns matches the second sorting order.
Actual results: The original sorting order selected is still used.

@bergice bergice force-pushed the pulls/1/dont-override-grid-state-with-get-parameter branch from e5c73d8 to 7594ef2 Compare November 17, 2020 23:21
@bergice bergice merged commit 208cb41 into silverstripe:1 Nov 18, 2020
@maxime-rainville maxime-rainville deleted the pulls/1/dont-override-grid-state-with-get-parameter branch November 18, 2020 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gridfield Better Buttons don't handle sorting and pagination
4 participants