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 Update page number in the state on reaching the first or the last… #10439

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Aug 5, 2022

Description

Update page number in the state on reaching the first or the last element in a list.
Keeping state in "better button" href for getting new set of elements after user reaches last or first element on a page by using navigation GridField Item.

Parent issue

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.

The overall approach looks good. I haven't tested this locally but it seems to make sense.
Couple of small changes - though note I was only looking at this from a "is the approach reasonable" point of view for now since it is in draft.

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/GridFieldDetailForm_ItemRequest.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/GridFieldDetailForm_ItemRequest.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
@sabina-talipova sabina-talipova force-pushed the pulls/4/better-button-keep-state branch 2 times, most recently from d029a58 to a8aa456 Compare August 9, 2022 02:40
@sabina-talipova sabina-talipova marked this pull request as ready for review August 9, 2022 03:35
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.

State issues

It doesn't looks like this works as expected - for example if I am on the test modeladmin and I search for "i" and go to page 3, then click on the top item and click previous, the link for the breadcrumb back button still takes me to page 3 instead of page 2. This video shows that happening:
PR doesn't solve issue
Looks like there are a few old bits of code getting in the way here, where the changes to gridfield state are being overridden by whatever is in the request:

  • delete these lines as this is done by GridField::addStateFromRequest() already
  • change these lines to $gridState = $this->gridField->getState(false);.

Move the logic

I know I said the approach was good, but I have since noticed that this can be called from getPreviousRecordID() and getNextRecordID() which are both public methods.
Please move the logic you added from getAdjacentRecordID() into the top of edit() - that way it should only happen when someone actually visits the form, not when calling public methods.

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/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4/better-button-keep-state branch 3 times, most recently from b4be0a6 to 5efb65e Compare August 17, 2022 01:35
@GuySartorelli
Copy link
Member

Also the PHPlinting CI job has failed.

@sabina-talipova sabina-talipova force-pushed the pulls/4/better-button-keep-state branch from 5efb65e to b2affc1 Compare August 17, 2022 05:08
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.

Looks good and works locally. Just a small change still outstanding from a previous review.

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4/better-button-keep-state branch from b2affc1 to c0b38fc Compare August 22, 2022 00:44
@GuySartorelli GuySartorelli merged commit a753173 into silverstripe:4 Aug 22, 2022
@GuySartorelli GuySartorelli deleted the pulls/4/better-button-keep-state branch August 22, 2022 01:47
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