neos-ui icon indicating copy to clipboard operation
neos-ui copied to clipboard

WIP: BUGFIX: Show if selectbox options mismatch with current value

Open mhsdesign opened this issue 1 year ago • 11 comments

solves: #3520

What I did

For the first and second case (mentioned in #3520) i could imagine adding a warning icon, a proper label and additionally allow to reset the value like:

image

And for the multi select box it could look like:

https://github.com/neos/neos-ui/assets/85400359/55a92ddc-a591-4fe0-9ee4-39712b2e6098

For a (slow) datasource it looks like:

https://github.com/neos/neos-ui/assets/85400359/890717cc-986e-42c4-a1e4-d74202fd6b2f

multiple: true

https://github.com/neos/neos-ui/assets/85400359/ef8c27a1-1693-481a-8b28-a7f1623165ac

How I did it

How to verify it

TODO:

  • [ ] ~translations~ Nope: to low level and this is just a bug fix. Wed need to refactor a lot. https://github.com/neos/neos-ui/pull/3526#issuecomment-1629025114
  • [x] backport to 7.3
  • [x] discuss icon choice
  • [x] discuss if my "simple" approach is enough or if the selectbox should highlight it differently -> yes stay simple for this bugfix
  • [ ] ~color icon and string red or orange~ Nope: this is just a bug fix. Wed need to refactor a lot. https://github.com/neos/neos-ui/pull/3526#issuecomment-1629025114
  • [ ] move this logic out of selectbox into usage

mhsdesign avatar Jun 12 '23 21:06 mhsdesign

Icon choice is okay IMO.

crydotsnake avatar Jun 13 '23 08:06 crydotsnake

I'd like to see a bit more warning color in the first example instead of only using the attention icon. Maybe the whole string including the icon could be red or orange. Otherwise, nice fix :)

tschoeki avatar Jun 20 '23 08:06 tschoeki

If you want, i can help you implementing the translations @mhsdesign

But maybe it would be better too backport this PR too 7.3 first.

crydotsnake avatar Jun 20 '23 09:06 crydotsnake

yes @crydotsnake id love your help ;) feel free to go ahead ^^

mhsdesign avatar Jun 20 '23 10:06 mhsdesign

yes @crydotsnake id love your help ;) feel free to go ahead ^^

Will try too take a Look at it today. But the backporting part to 7.3 would be better in your hands i believe ;) Wouldnt it be better to do this first?

crydotsnake avatar Jun 20 '23 10:06 crydotsnake

Also: a merge conflict should come up with #3485 because of the translations. But this should be easy too fix.

crydotsnake avatar Jun 20 '23 10:06 crydotsnake

The failing unit test is actually right.

In case youre using a datasource, it will first show for a millisecond or sth that the value is invalid, until the datasource is resolved. This is not ideal user experience in cases where this "flashing" might be actually visible -> for example if the datasource is complex or connection is slow.

https://github.com/neos/neos-ui/blob/033b086a2e66c745fd36a78c3193941e49ee31fd/packages/neos-ui-editors/src/Editors/SelectBox/index.spec.js#L220

edit: tested it, we instead already show a loading spinner, so this is not a real problem.

edit2: no with multiple: true this is indeed a problem:

https://github.com/neos/neos-ui/assets/85400359/3fb6c72d-86c2-4fb4-8aec-4c47f27ddbb4

fixed it:

https://github.com/neos/neos-ui/assets/85400359/3c9977e6-880d-4801-a89b-87b207bed134

mhsdesign avatar Jun 20 '23 18:06 mhsdesign

while i agree it would be nice to have a more explicit color and translations, i dont think its worth the additional complexity under which the SelectBox and MultiSelectBox would have to suffer.

We currently have no concept at all about validating that the editor matches the value and the type, and it might be the wrong place to start with the most complex editor: the select box.

Other editors might face similar problems but a changed datasource might be the most realistic (it happened in a client project)

So for this mini bugfix patch i would either:

A: keep it as is B: keep it visually as is, but show an console.warning C: use the proposed solution to show the label as invalid: "foo"

in a longer run we need a concept for every editor, and i could imagine solving it nicely via the validation api.

mhsdesign avatar Jul 10 '23 13:07 mhsdesign

Is this now still "WIP" or not?

Sebobo avatar Jul 11 '23 08:07 Sebobo

I assume this is ready because of: SCR-20230711-jthv

Visually its still the same as before. Wasn't the idea that the selector is outlined in orange afterwards? Or have I misunderstood something?

crydotsnake avatar Jul 11 '23 08:07 crydotsnake

Is this now still "WIP" or not?

im still not sure if this is the right approach. @grebaldi and me have prototyped a little thing to allow any editor to report an implausible value which will prevent the editor from being rendered and will show a "reset value" button with the note that the editor is unable to understand this value.

https://github.com/neos/neos-ui/pull/3580

3580 might be a more general solution while this approach a bit more welcome for editors as they can explicitly chose which values to remove and keep from a multi-select box.

This topic just needs some discussion and experimentation. And is WIP for now ;)

mhsdesign avatar Jul 12 '23 09:07 mhsdesign

If we go this route the implementation should be in the integration in the select box and probably not directly in it

mhsdesign avatar Mar 11 '24 16:03 mhsdesign

Replaced by https://github.com/neos/neos-ui/pull/3745 which features translation and implements this logic outside of the select box, in the integration layer

mhsdesign avatar Jun 24 '24 09:06 mhsdesign