Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jun 12, 2023

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:

Bildschirmaufnahme.2023-06-12.um.23.23.53.mov

For a (slow) datasource it looks like:

Bildschirmaufnahme.2023-07-10.um.15.04.19.mov

multiple: true

Bildschirmaufnahme.2023-07-11.um.09.35.15.mov

How I did it

How to verify it

TODO:

@crydotsnake
Copy link
Member

Icon choice is okay IMO.

@tschoeki
Copy link

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 :)

@crydotsnake
Copy link
Member

crydotsnake commented Jun 20, 2023

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.

@mhsdesign
Copy link
Member Author

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

@crydotsnake
Copy link
Member

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 crydotsnake added Bug Label to mark the change as bugfix 7.3 and removed 8.3 labels Jun 20, 2023
@crydotsnake
Copy link
Member

crydotsnake commented Jun 20, 2023

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

@mhsdesign
Copy link
Member Author

mhsdesign commented Jun 20, 2023

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.

expect(multiselectLabels(component)).toEqual([]);

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:

Bildschirmaufnahme.2023-07-11.um.09.27.47.mov

fixed it:

Bildschirmaufnahme.2023-07-11.um.09.34.13.mov

@crydotsnake crydotsnake force-pushed the bugfix/3520-selectbox-current-value-doesnt-match-options branch from 033b086 to 3d7d7f7 Compare July 5, 2023 12:36
@github-actions github-actions bot added the 8.3 label Jul 5, 2023
@crydotsnake crydotsnake changed the base branch from 8.3 to 7.3 July 5, 2023 12:37
@mhsdesign mhsdesign force-pushed the bugfix/3520-selectbox-current-value-doesnt-match-options branch from 3d7d7f7 to 823f2fc Compare July 10, 2023 13:11
@mhsdesign
Copy link
Member Author

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 mhsdesign marked this pull request as ready for review July 11, 2023 07:44
@Sebobo
Copy link
Member

Sebobo commented Jul 11, 2023

Is this now still "WIP" or not?

@crydotsnake
Copy link
Member

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?

@mhsdesign mhsdesign marked this pull request as draft July 11, 2023 09:57
@mhsdesign
Copy link
Member Author

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.

#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
Copy link
Member Author

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

@mhsdesign
Copy link
Member Author

Replaced by #3745 which features translation and implements this logic outside of the select box, in the integration layer

@mhsdesign mhsdesign closed this Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.3 Bug Label to mark the change as bugfix next-patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Select Box show as empty when current value doesnt match anymore the values
4 participants