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

BUGFIX: Select box shows invalid value #3745

Merged
merged 12 commits into from
Jul 2, 2024
Merged

Conversation

73nici
Copy link
Collaborator

@73nici 73nici commented Mar 13, 2024

replaces the initial solution #3526 with a refactored approach applied from outside the select-box to reduce the logic there and add translation for the label.

Resolves: #3520

What I did

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

image

And for the multi select box it will look like this, ensuring the sorting will still work

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

and for multiple: true it looks like:

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

Related discussion #3526 (comment)

@github-actions github-actions bot added Bug Label to mark the change as bugfix 8.4 labels Mar 13, 2024
@73nici 73nici changed the base branch from 8.4 to 8.3 March 14, 2024 08:16
@github-actions github-actions bot added 8.3 and removed 8.4 labels Mar 14, 2024
@mhsdesign
Copy link
Member

mhsdesign commented Jun 20, 2024

Old description from Niclas

Currently the selectbox shows the "choose" placeholder, if an invalid option is selected. This is misleading, cause a value is still selected and therefore can be used.

If the current selected value is not a valid option, the placeholder changes to "invalid value" without the value being updated.
To check this, remove a selected value from a select box. After reloading, the placeholder should read "invalid value".

Fixes #3520

New behaviour:
image

Old behaviour:
image

Hi Niclas ;)

i adjusted your pr a bit to what we discussed at the sprint i guess... following todos are still todo:

  • transform the helper to ts
  • add unit tests for it
  • console error when used standalone as component to indicate there are missing values -> not needed for now
  • update description
  • fix index when dragging and dropping
  • adjust label wording?

@Sebobo Sebobo self-requested a review June 20, 2024 11:55
@mhsdesign mhsdesign marked this pull request as ready for review June 24, 2024 09:15
@mhsdesign
Copy link
Member

mhsdesign commented Jun 24, 2024

I added some test for processSelectBoxOptions and also run them against the legacy implementation to make sure the behaviour matches. The result ist promising, only the tests for the new features fail (creates missing option). So the behaviour is correct ;)

FAIL  packages/neos-ui-editors/src/Editors/SelectBox/selectBoxHelpers.spec.ts
  processSelectBoxOptions
    ✓ transforms an associative array with labels to list of objects (1 ms)
    ✓ keeps valid shape of list of objects intact
    ✓ overrules the array key with the explicit value
    ✓ uses numeric string array key for list of objects
    ✓ omits entries that are invalid and empty (1 ms)
    ✕ creates missing option for unmatched string value (2 ms)
    ✕ creates missing options for unmatched additional array value (1 ms)
    ✕ creates missing options for unmatched additional multiple array values (1 ms)
    ✓ ignored current value being null or undefined and doesnt create missing option

@mhsdesign
Copy link
Member

e2e failure is currently related:

 ✓ Create a text node in a new container element at the correct position

 Create new nodes
 ✖ Check ClientEval for dependencies between properties of NodeTypes in Creation Dialog

   1) AssertionError: expected [ Array(6) ] to deeply equal [ Array(5) ]

      + expected - actual

         {
           label: 'label_9'
           value: 9
         }
      -  {
      -    icon: 'exclamation-triangle'
      -    label: 'Invalid value: ""'
      -    value: ''
      -  }
       ]


      Browser: Firefox 127.0 / Windows 10 (https://app.saucelabs.com/tests/8ce1a59a62ca48fcb7c99bae4d08ac28)

         41 |            {'label': 'even', value: 'even'}
         42 |        ])
         43 |
         44 |    await t
         45 |        .expect(dependingPropertySelectBox.props.value).eql('')
       > 46 |        .expect(dependingPropertySelectBox.props.options).eql([
         47 |            {label: 'label_1', value: 1},
         48 |            {label: 'label_3', value: 3},
         49 |            {label: 'label_5', value: 5},
         50 |            {label: 'label_7', value: 7},
         51 |            {label: 'label_9', value: 9}

         at <anonymous> (/home/circleci/app/Packages/Application/Neos.Neos.Ui/Tests/IntegrationTests/Fixtures/1Dimension/createNewNodes.e2e.js:46:59)
         at asyncGeneratorStep (/home/circleci/app/Packages/Application/Neos.Neos.Ui/Tests/IntegrationTests/Fixtures/1Dimension/createNewNodes.e2e.js:4:38)
         at _next (/home/circleci/app/Packages/Application/Neos.Neos.Ui/Tests/IntegrationTests/Fixtures/1Dimension/createNewNodes.e2e.js:4:38)

@mhsdesign
Copy link
Member

I dont understand the test completely, how can there be an empty string in an integer nodetype property???

.expect(dependingPropertySelectBox.props.value).eql('')

i guess empty string means unset??? we should check on that but ill adjust the "create magic select box option" logic to ignore empty string

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on my machine, thx!
Even the createNew thing which is nowhere documented I think.
I also have to add those options to my NodeTypes schema.

@73nici
Copy link
Collaborator Author

73nici commented Jun 28, 2024

Hey @mhsdesign I think fixed the CI :). Only a snapshot update for selectbox was needed.

@73nici 73nici requested a review from Sebobo June 28, 2024 18:21
@mhsdesign mhsdesign requested a review from grebaldi July 1, 2024 08:45
@Sebobo Sebobo merged commit 1d7a9e9 into neos:8.3 Jul 2, 2024
9 checks passed
@73nici 73nici deleted the SelectBoxInvalidValues branch July 12, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 Bug Label to mark the change as bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants