Mark all parent accordions/tabs if validation exception of control inside

[mkrecek234]

Introduces #1728

Demo can be seen by opening demos/form/form.php demo, pressing save on the second form ("but very flexibly"). The field status integer not nullable will then not validate and you can see that the Accordion is automatically opened.

ui/demos/form/form.php

Line 60 in 621d33f

$accordionLayout = $form->layout->addSubLayout([\Atk4\Ui\Form\Layout\Section\Accordion::class]);

[mvorisek]

this smells to me - getOwner()->getOwner() and below even triple times it seems it is single purpose code only and for more layout nesting it will not work

also, ideally it should not be handled in Form, as form should not care about it's childs too much

also Behat test should be added, so this code is covered - otherwise the logic might break soon - once you will satisfy all CI tests, you should see this in CodeCov report

[mkrecek234]

getOwner multiple times also is used in other view, like Crud. You are right, nested Accordions are then not supported - but since today it just won't work at all, this is a huge improvement to usability. And to me, nested Accordions in forms really is an edge case which would not be priority to me to support yet.

[mvorisek]

Also, what would happen if you have errors in more tabs/accordions? I would expect some error icon on the tabs/accordion menus - the "first accordion error" might be an error comming from other tab/accordion.

[mkrecek234]

Certainly this is not the perfect solution, also Tabs in Forms are not covered. Do you have a better solution in mind? This is clearly a minor field which would not justify to refactor the whole rendering logic, but is a critical situation for users - if there is an error only with a closed accordion, the user does not get any feedback. This would be a pragmatic solution, and unfortunately Ui does not offer to add a badge to the closed Accordion...

If there is an error also in another Accordion, at next save it would open that second Accordion.

[mvorisek]

The main UX problem is user must be aware of all errors. Currently, user must click thru all sections which is definitely an issue. However, when the first section with an error is opened, the problem remains unsolved.

I would like the error badge instead which can also cascade up easily.

A badge/icon is already used in:

in demos/layout/layout-panel.php demo

Opening first error might not be even wanted, if there are more errors on more pages and user is currently on the non-first page /w error he just entered.

[mkrecek234]

@mvorisek To my knowledge the Accordion Section in Fomantic UI does not easily allow to add an icon, at least not that I know of or that Atk4 supports it. Sure this would be better - in the current state however, it is a big issue - the user believes he saved, because he does not see ANY error message if only a field inside an accordion is invalid. We had these issues with users many times. Can we cut this into two steps

1. We just open the first Accordion for now, so the error is displayed in the incorrect field

and later, - if we there is an easy solution with Fomantic Ui to add icons to Accordion Sections - we

2. mark all sections with errors with that icon.

Even in the scenario of multiple sections to have errors, you want the first section to be opened automatically. One click saved, and much easier to grasp for the user what he is expected to do.

[mvorisek]

I fully confirm this issue and agree it must be fixed.

I however do not agree the implementation with nested getOwner is valid and even belive it can crash the App, if 2/3+ owners are not defined. (imagine Form with Control only and no App/Form Layout)

As discussed in previous posts, the "opening the 1st section" solution is not a general solution and is Accordion specific. Component consistency is important and no fix should be merged until it fixes Tabs too.

Also, while testing this PR, I hit #1821. So this issue should be fixed first. Repro steps: save form and then click section header to expand (you can click multiple times to open/close) and you will see messages in DOM browser console.

[mkrecek234]

May be we use jQuery to find all parent accordion sections to get rid of the triple getOwner logic, see here: https://stackoverflow.com/questions/5333426/how-to-find-a-parent-with-a-known-class-in-jquery

We could also search for tabs or only for the warning icon class.

This would open also all nested parents in a row/ mark all nested parents with a warning sign ‘d.parents('.a').attr('id')‘

Or even better all parents up to/inside current form: https://api.jquery.com/parentsUntil/ so we do not go outside current form.

[mvorisek]

yes, but in PHP, no need for jQuery, there are even some helper method for it present

[mkrecek234]

yes, but in PHP, no need for jQuery, there are even some helper method for it present

I know, but we could make use of the jQuery function - calling it through the Atk4 jQuery helper code. Do you like the logic in order to cover nested warning icons/tabs/accordions?

[mvorisek]

yes, but in PHP, no need for jQuery, there are even some helper method for it present

I know, but we could make use of the jQuery function - calling it through the Atk4 jQuery helper code. Do you like the logic in order to cover nested warning icons/tabs/accordions?

yes, I meant View::getClosestOwner method

yes, nested, multiple errors...

[mkrecek234]

Warning icon should be red as all validation errors. The orange one is used for canLeave=true on changes