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 Return PJAX responses from gridfield edit forms #11206

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Apr 23, 2024

Description

Adds a PJAX response negotiator to GridFieldDetailForm_ItemRequest so that javascript which is expecting PJAX responses will get what it's expecting.

Note there's a kitchen sink CI run linked in the main issue

Manual testing steps

  1. Add silverstripe/frameworktest to a project and make sure all of the operations work as expected for companies (which are versioned) and employees (which are not versioned) in "Test ModelAdmin"
  2. Add an elemental area to the Company class (yml config snippet below), add a block with an upload field (e.g silverstripe/elemental-fileblock) and make sure you can upload a file to the block and save with the page save button.
SilverStripe\FrameworkTest\Model\Company:
  extensions:
    - DNADesign\Elemental\Extensions\ElementalAreasExtension
  has_one:
    ElementalArea: 'DNADesign\Elemental\Models\ElementalArea'
  owns:
    - 'ElementalArea'
  cascade_duplicates:
    - 'ElementalArea'

Issues

@GuySartorelli
Copy link
Member Author

Note that while this is being implemented to facilitate fixing a bug, the approach is somewhat more risky in terms of chance of regressions than I feel comfortable with in a patch release - hence targeting 5 instead of 5.2.

@@ -170,7 +171,7 @@ public function edit($request)
])->renderWith($this->getTemplates());

if ($request->isAjax()) {
return $return;
return $this->getResponseNegotiator($return)->respond($request);
Copy link
Member Author

@GuySartorelli GuySartorelli Apr 23, 2024

Choose a reason for hiding this comment

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

Doing this here means it applies for the save and publish actions, with a relatively low chance of affecting custom actions people add.

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Apr 23, 2024

I went with this approach because admin uses PJAX for the form submission and elemental expects a PJAX response in return. It doesn't make a distinction between different admin sections or different contexts for the form, nor should it.

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

There's also and admin behat failure on the sink run https://github.com/creative-commoners/recipe-kitchen-sink/actions/runs/8808259049/job/24177023040 - I've tried rerunning once though the same error has appeared so it's probably legit

src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
src/Forms/GridField/GridFieldDetailForm_ItemRequest.php Outdated Show resolved Hide resolved
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Tested locally, works good. CI failures are expected

@emteknetnz emteknetnz merged commit 142a318 into silverstripe:5 Apr 26, 2024
5 of 15 checks passed
@emteknetnz emteknetnz deleted the pulls/5/pjax-in-gridfield branch April 26, 2024 05:13
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