neos-ui
neos-ui copied to clipboard
WIP: BUGFIX: Show if selectbox options mismatch with current value
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:
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
Icon choice is okay IMO.
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 :)
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.
yes @crydotsnake id love your help ;) feel free to go ahead ^^
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?
Also: a merge conflict should come up with #3485 because of the translations. But this should be easy too fix.
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
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.
Is this now still "WIP" or not?
I assume this is ready because of:
Visually its still the same as before. Wasn't the idea that the selector is outlined in orange afterwards? Or have I misunderstood something?
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 ;)
If we go this route the implementation should be in the integration in the select box and probably not directly in it
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