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

[Components - ToggleGroupControl]: Fix visual state when no option is selected #35545

Conversation

ntsekouras
Copy link
Contributor

Fixes: #35383

When we pass a value to ToggleGroupControl that doesn't exist in the available options the backdrop should not be visible.

Testing instructions

  1. Run storybook
  2. Check the With Reset story
  3. Observe that the backdrop is not visible when we reset.

The case is being discussed here: https://github.com/WordPress/gutenberg/pull/35395/files#r726460400 and previous mention to the existing code about targetNode is here.

The thing is that this is probably related to Reakit and was related to the fix here: #35409 (that got removed now). The problem is probably that if we have a value in Reakit state that doesn't exist in our options, the currentId is filled with the first available option, which is not what we want. Reakit docs state that if we pass null the currentId will not be set, but with some tests of mine, it still had the first option as value.

--cc @sRahul-00

@ntsekouras ntsekouras added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Oct 12, 2021
@ntsekouras ntsekouras self-assigned this Oct 12, 2021
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Testing the Storybook example, the problem seems to be solved.

I have a question though: with this fix, we're rendering BackdropView with a width of 0. Should we instead look into not rendering that component at all?

@ntsekouras
Copy link
Contributor Author

I have a question though: with this fix, we're rendering BackdropView with a width of 0. Should we instead look into not rendering that component at all, instead?

I updated the code 😄

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Thank you for Changes LGTM!

I left a small comment, feel free to address it and merge after that.

@talldan talldan merged commit a304131 into trunk Oct 14, 2021
@talldan talldan deleted the fix/toggle-group-control-visual-state-when-value-not-exists-in-options branch October 14, 2021 02:51
@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ToggleGroupControl: Visual state of an undefined value is inconsistent
3 participants