search

silverstripe / silverstripe-elemental

Create pages in Silverstripe CMS using content blocks
http://dna.co.nz
BSD 3-Clause "New" or "Revised" License
109 stars 115 forks source link

NEW Inline save all rendered element forms on parent form submit #1214

Closed emteknetnz closed 1 month ago

emteknetnz commented 2 months ago

Issue https://github.com/silverstripe/silverstripe-elemental/issues/1215

Requires https://github.com/silverstripe/silverstripe-admin/pull/1792 and https://github.com/silverstripe/silverstripe-behat-extension/pull/277 and https://github.com/silverstripe/silverstripe-frameworktest/pull/193 and https://github.com/silverstripe/silverstripe-framework/pull/11301

What this PR does

GuySartorelli commented 2 months ago

Problems encountered when testing locally:

There's no feedback

  1. Open a block
  2. Make an edit which makes the block invalid
  3. Close the block
  4. Click "Save" on the page

There's nothing at all indicating that something went wrong. No toast, and the block doesn't open. It looks like the page save is just broken but in reality there's a validation error I'm not being told about.

There should definitely be a toast message, and ideally the block should open so that I can immediately see the validation error message without opening all the blocks to try find it.

This is partially https://github.com/silverstripe/silverstripe-elemental/issues/1177 which is meant to be fixed by this PR.

Blocks get double saved - which turns them back to "modified"

  1. Have some valid blocks.
  2. Click "Publish" on the page. The blocks should all be in a published state.
  3. Open one of the blocks. Made no modifications.
  4. Click "Save" on the page

The block is now "Modified" even though I didn't make any changes! Note that the page itself isn't "modified", so this isn't just how the CMS treats a save that happens after a publish.

validate() method validation still gets confused with fields on the page

  1. Implement a validate() method on a block which adds this: $result->addFieldError('Title', 'Nah bro');
  2. Do not open any blocks
  3. Click "Publish" on the page (doesn't happen with save)

The "Nah bro" message appears underneath the page title field. This is https://github.com/silverstripe/silverstripe-elemental/issues/1183 which is meant to be fixed by this PR.

GuySartorelli commented 1 month ago

Note that I still have the "no feedback" and "Blocks get double saved - which turns them back to "modified"" issues as described above.

Note that the "no feedback" problem only happens with form validation (i.e. getCMSCompositeValidator() - I specifically add a RequiredFields validator for the Title field). Using the validate() method I do get a toast and the block does open as expected.

emteknetnz commented 1 month ago

I've solved the "Blocks get double saved - which turns them back to "modified" issue - change here

For the "no feedback" issue with RequiredFields ... I don't know how to resolve this one without globally removing client-side validation (which admittedly would simplify things, but we probably shouldn't. Though I also won't really mind :-).

The issue is that RequiredFields has client-side (pure js) validation which prevents an XHR request from being sent to the server in the first place. The 'feedback' of popping opening the element happens as part of the XHR response which is formSchema formatted and contains the validation errors.

However with the client-side validation there's nothing "returned" to trigger opening the element. If we did globally remove client-side validation then it'll make an XHR request and fail validation on the server and behave the same UX wise as DataObject->validate() failing

I think the client-side validation is passed in as a callback into redux-form ... somehow (this maybe?). I don't think there's anyway to somehow extract a 'response' from it, particularly as we're remote-submitting the form which does not return a promise

So I guess, options are: a) Globally disable client-side validation b) Hack is something kind of nasty that emits an event from Validator.js and we listen for it in Element.js .. somehow .. c) Just live with the bad UX

GuySartorelli commented 1 month ago

Just live with the bad UX

I don't think that's really an option, since RequiredFields is the only validator we offer out-of-the-box so it's presumably used a lot. Not having any feedback at all about why submitting a form did nothing is really bad IMO. If I was a content author and clicking "save" seemed to do literally nothing, I'd be contacting my vendor and reporting a critical bug. If there was either a toast or the block popped open, it wouldn't be so bad.

It's interesting to note that a RequiredFields validator for any field on the page directly doesn't have this same behaviour - in that case a toast is triggered.

Is that front-end validation JS able to trigger a toast? That would resolve the problem well enough for now, I think. Better if the block opens, but at least if there's a toast it prompts the content author to start looking for a validation error message as opposed to just thinking something's not working the way it should be.

emteknetnz commented 1 month ago

I've the validation fixed it by hardcoding something into admin to disable client-side validation for elemental specifically - https://github.com/silverstripe/silverstripe-admin/pull/1792/files#diff-cc40afd32326a7c9ddea47afd5623f5dd54abfe99db517c1f6e72ca43337a4cbR73

It's obviously not the prettiest, though the UX experience is now much better.

Note that I'm pretty sure this change will require updating a behat test that expects client side validation, so rerun tests for this is the admin PR is merged

GuySartorelli commented 1 month ago

There are some unresolved comments:

GuySartorelli commented 1 month ago

Behat failing

emteknetnz commented 1 month ago

Needed to tag 5.4.0 of behat-extension, have done that an re-run, working now