FIX Retain value that failed validation on client side #930

emteknetnz · 2021-08-26T02:11:25Z

Requires this PR silverstripe/silverstripe-admin#1246 to get the ValidationResult client-side

Video

~~https://www.youtube.com/watch?v=NvWJHFSHijo~~
~~custom "required" validator on content block title~~
~~RequiredFields validator on a custom page field~~
~~Page has multiple tabs that show invalid icons~~

Make sure you squash commits on merge

There are two commits so it is clear what was done originally vs what has been done more recently. The original commit may be useful for historical purposes but when merging, this PR should be merged with squashed commits.

CI

The CI won't go green until silverstripe/silverstripe-admin#1246 is merged in, so I've run an installer CI run: https://github.com/creative-commoners/silverstripe-installer/actions/runs/4985675862

Note that the failed unit test in the installer CI run is unrelated to this work.

Issue

#764

clarkepaul · 2021-09-02T03:44:56Z

@emteknetnz A few UX type things which you might be able to fix as part of this issue but might be better placed as separate issues.

Can we add some more spacing to tabs that have an error? the icon is so close the the following tab that its hard to see which the icon is related to (I've asked this in the past but was told it wasn't possible so trying my luck again).
Is it possible to combine the two notifications (including block info) so they just appear at the top of the page? could be something like this
The notification at the top is too close to the second row of tabs, tabs should have a 52px height (53px with border), notifications should have a margin below them.

7. If pt.2 isn't possible, my suggestion for the text in block errors. I'm assuming you can have multiple errors so opting to bullet point them—even with one error.

A good UX experience would be highlighting blocks with some sort of visual indication which have error.

emteknetnz · 2021-09-02T23:16:53Z

yep
is technically possible, though its implementation is honestly quite hackish. Also I think it's wrong to put field errors inside the general form error. Also it will be confusing for users that have 2 or more elemental areas, I think warnings above the relevant elemental area would be better.
can do
that's doable

highlighting blocks with some sort of visual indication which have error

Unfortunately not possible without very hacky code, because it would be jquery updating a rendered react component

clarkepaul · 2021-09-08T03:50:21Z

"I think warnings above the relevant elemental area would be better."

If there wasn't another competing error at the top of the page which is kinda repeating information, and also close in proximity perhaps. Watching the video it's very in your face when you have multiple errors showing with multiple error notifications as well, and then there are instances where the field shows an error too. Just think this might be doing more harm than good having so many errors present.

I understand there might be technical limitations which mean the experience is sub-optimal especially with #4 not being possible.

I think we absolutely need an error to be sticky near the top incase people might miss the fading toast notifications. An idea (might not be the right thing?), we might have the possibility to use sticky toasts with a dismiss (x). Then the top alert info could be transferred into the toast. I don't think we've used the Toast dismiss yet but this could be an okay scenario to use it.

Also I think it's wrong to put field errors inside the general form error.

This was actually seen as best practice years ago when lots of errors can present (pretty sure it still would be), means you can assess all the issues in one place to get a sense of what needs to be done. Helps to prevent people correcting one field and then try to save the form again but only to get the following issue for each repeated save.

Keen to hear your thoughts on moving the top alert info to a sticky toast @emteknetnz

GuySartorelli · 2023-05-15T02:03:33Z

yarn.lock

Not sure why these changes are here but given this is an automatically generated file I'm inclined to keep them

GuySartorelli · 2023-05-15T05:13:40Z

tests/Behat/features/edit-block-element.feature

Changes to this file are to make the tests more robust, mirroring the equivalent assertions in the new validation failed feature tests.

GuySartorelli · 2023-05-15T05:16:09Z

tests/Behat/Context/FeatureContext.php

-        } elseif ($negate) {
-            $this->assertFieldNotContains($field, $content);
+            return;
+        }
+
+        $actual = (string) $fieldElem->getValue();
+        $regex = '/^' . preg_quote($content, '/') . '$/ui';
+
+        if ($negate) {
+            $message = sprintf('The field "%s" value is "%s", but "%s" expected.', $field, $actual, $content);
+            Assert::isTrue((bool) preg_match($regex, $actual), $message);
        } else {
-            $this->assertFieldContains($field, $content);
+            $message = sprintf('The field "%s" value is "%s", but it should not be.', $field, $actual);
+            Assert::isFalse((bool) preg_match($regex, $actual), $message);
        }


assertFieldNotContains() and assertFieldContains() failed because the $field variable wasn't a string (so obviously this part of the code just never worked but was never used).

The new assertions mirror what assertFieldNotContains() and assertFieldContains() methods would have done. I don't use those directly because those will look fo the fields on the page, not on the block - and since we already have a reference to the field on the block, it's more sensible to just check against that directly here.

GuySartorelli · 2023-05-15T05:16:43Z

client/src/legacy/ElementEditor/entwine.js

+        } else {
+          // No validation result returned
+          // assume everything is valid and reset stores so data is refetched.
+          resetStores();
+        }


This is needed for compatibility with versions of silverstripe/admin that don't have the accompanying PR.

I think I'd prefer to just tighten the constraint within elementals composer.json to ^1.13.2

Done. CI will break until admin is tagged.

GuySartorelli · 2023-05-15T05:18:46Z

client/src/legacy/ElementEditor/entwine.js

+      // Reset the store if the user navigates to a different part of the CMS
+      // or after submission if there are no validation errors
+      if (!$('.cms-edit-form').data('hasValidationErrors')) {
+        resetStores();
+      }


The condition doesn't explicitly test for anything to see if it's swapping to a new page - but it doesn't need to. This condition evaluates to true when navigating to a new page. I've confirmed this under the following scenarios:

No form submission has happened yet. I click to a different page.

A form submission happened and there were no validation errors. I click to a different page.

A form submission happened and there were validation errors. I click to a different page.

emteknetnz

I clicked "Comment" rather than "Request changes" because I'm unable to request changes since I'm the original author on this PR

emteknetnz · 2023-05-15T23:08:25Z

client/src/legacy/ElementEditor/entwine.js

+        } else {
+          // No validation result returned
+          // assume everything is valid and reset stores so data is refetched.
+          resetStores();
+        }


I think I'd prefer to just tighten the constraint within elementals composer.json to ^1.13.2

Getting validation errors to display nicely on blocks is out of scope - we're just making it so if there _are_ validation errors, block content is not lost.

emteknetnz

APPROVE
(I cannot tick approve because I'm the original author on the PR)

Tested locally, it's certainly a big approvement though the UX still isn't quite there

When I make inline edits to a content block such as changing the title and then save the page and fail page validation, the content block edits are retained, though the when the page edit form is refreshed the content block title appears unchanged in a closed state. When you open the content block to inline edit it the changes are now applied.

That said, it is now retaining the edits which is a great improvement

emteknetnz force-pushed the pulls/4/preview-non-editable branch from 9da12b8 to b543861 Compare August 26, 2021 05:52

emteknetnz mentioned this pull request Aug 26, 2021

CMS users lose all blocks when attempting to save page with validation errors #764

Closed

1 task

emteknetnz force-pushed the pulls/4/preview-non-editable branch 4 times, most recently from a99f5bb to 6d8e29f Compare August 27, 2021 06:13

emteknetnz changed the title ~~ENH Show validation errors next to fields~~ FIX Retain value that failed validation on client side Aug 28, 2021

emteknetnz force-pushed the pulls/4/preview-non-editable branch 10 times, most recently from 2da5c6f to 65e2d6c Compare September 1, 2021 04:36

FIX Retain value that failed validation on client side

fc19dc9

GuySartorelli force-pushed the pulls/4/preview-non-editable branch from 65e2d6c to fc19dc9 Compare May 10, 2023 21:55

GuySartorelli changed the base branch from 4 to 4.11 May 10, 2023 21:56

GuySartorelli reviewed May 15, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/preview-non-editable branch 4 times, most recently from 11d1d4f to e01463a Compare May 15, 2023 05:12

GuySartorelli reviewed May 15, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/4/preview-non-editable branch from e01463a to 7b3e94c Compare May 15, 2023 05:20

GuySartorelli mentioned this pull request May 15, 2023

Reduce reliance on frameworktest for behat tests which require test extension classes silverstripe/silverstripe-frameworktest#140

Open

GuySartorelli force-pushed the pulls/4/preview-non-editable branch from 7b3e94c to c473c1e Compare May 15, 2023 22:13

GuySartorelli marked this pull request as ready for review May 15, 2023 22:50

emteknetnz commented May 15, 2023

View reviewed changes

Focus PR back to original scope

2c32dc8

Getting validation errors to display nicely on blocks is out of scope - we're just making it so if there _are_ validation errors, block content is not lost.

GuySartorelli force-pushed the pulls/4/preview-non-editable branch from c473c1e to 2c32dc8 Compare May 15, 2023 23:15

GuySartorelli mentioned this pull request May 16, 2023

ENH Pass form validation result to client silverstripe/silverstripe-admin#1246

Merged

emteknetnz commented May 16, 2023

View reviewed changes

GuySartorelli merged commit 43d078f into silverstripe:4.11 May 16, 2023

GuySartorelli deleted the pulls/4/preview-non-editable branch May 16, 2023 23:34

GuySartorelli mentioned this pull request May 17, 2023

Broken builds in 5.0 silverstripe/.github#47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Retain value that failed validation on client side #930

FIX Retain value that failed validation on client side #930

emteknetnz commented Aug 26, 2021 •

edited by GuySartorelli

Loading

clarkepaul commented Sep 2, 2021 •

edited

Loading

emteknetnz commented Sep 2, 2021

clarkepaul commented Sep 8, 2021

GuySartorelli May 15, 2023

GuySartorelli May 15, 2023

GuySartorelli May 15, 2023

GuySartorelli May 15, 2023

emteknetnz May 15, 2023

GuySartorelli May 15, 2023

GuySartorelli May 15, 2023 •

edited

Loading

emteknetnz left a comment

emteknetnz May 15, 2023

emteknetnz left a comment

FIX Retain value that failed validation on client side #930

FIX Retain value that failed validation on client side #930

Conversation

emteknetnz commented Aug 26, 2021 • edited by GuySartorelli Loading

Video

Make sure you squash commits on merge

CI

Issue

clarkepaul commented Sep 2, 2021 • edited Loading

emteknetnz commented Sep 2, 2021

clarkepaul commented Sep 8, 2021

GuySartorelli May 15, 2023

Choose a reason for hiding this comment

GuySartorelli May 15, 2023

Choose a reason for hiding this comment

GuySartorelli May 15, 2023

Choose a reason for hiding this comment

GuySartorelli May 15, 2023

Choose a reason for hiding this comment

emteknetnz May 15, 2023

Choose a reason for hiding this comment

GuySartorelli May 15, 2023

Choose a reason for hiding this comment

GuySartorelli May 15, 2023 • edited Loading

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

emteknetnz May 15, 2023

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

emteknetnz commented Aug 26, 2021 •

edited by GuySartorelli

Loading

clarkepaul commented Sep 2, 2021 •

edited

Loading

GuySartorelli May 15, 2023 •

edited

Loading