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

Fixed condition to show separator correctly #35409

Conversation

amustaque97
Copy link
Member

Description

Modified condition to toggle separator when the current button is active and
showSeparator value is false. This condition will handle both undefined or
any random value which is not present in the toggle-group-control-option.

https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/toggle-group-control/toggle-group-control-button.tsx#L59

How has this been tested?

  • Checkout to my PR branch.
  • Run storybook using npm run storybook:dev
  • Use following code
 <ToggleGroupControl value={undefined} >
    <ToggleGroupControlOption
      key="foo"
      label="foo"
      value="foo"
    />
    <ToggleGroupControlOption
      key="bar"
      label="bar"
      value="bar"
    />
  </ToggleGroupControl>
  • Navigate to ToggleGroupControl -> default
  • Verify Changes ie it will show separator for all the buttons/options

Screenshots

image

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

related to #35382
cc @ntsekouras

Modified condition to toggle separator when current button is active and
showSeparator value is false. This condition will handle both undefined or
any random value which is not present in the toggle-group-control-option
Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @amustaque97 !

I'm afraid the problem has more nuances than this. You can test yourself that the separator is shown always except after the active item. It will be easier to notice if you change this style to something like red: https://github.com/WordPress/gutenberg/blob/trunk/packages/components/src/toggle-group-control/styles.ts#L111

This is the function that needs changing and has to do with our checks of currentId and the internal handling of it from Reakit

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @amustaque97 🎉
It was a pleasure collaborating with you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants