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: polish ToggleGroupControl #35600

Merged
merged 7 commits into from
Oct 14, 2021

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Oct 13, 2021

Description

This PR is a refactor of the ToggleGroupControl component:

Tracking changes commit-by-commit may offer an easier way to review this PR.

I will potentially follow-up with another PR splitting the code in each component.tsx into a separate hook.ts file, as per the guidelines

How has this been tested?

There should be no runtime behavioural changes:

  • Project should build with no TypeScript errors
  • Tests should pass
  • Storybook examples should work the same way as in production

Types of changes

Refactor / tidy up

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

@ciampo ciampo self-assigned this Oct 13, 2021
@ciampo ciampo added [Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Developer Documentation Documentation for developers labels Oct 13, 2021
@ciampo ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Oct 13, 2021
const { ButtonContentView, LabelPlaceholderView, LabelView } = styles;

function ToggleGroupControlOption(
props: WordPressComponentProps< ToggleGroupControlOptionProps, 'button' >,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to update this type from

WordPressComponentProps< ToggleGroupControlOptionProps, 'input' >

to

WordPressComponentProps< ToggleGroupControlOptionProps, 'button' >

because otherwise TypeScript would throw an error on the Radio component, since it has the as="button" prop.

@ciampo ciampo force-pushed the refactor/components-toggle-group-control-option-button branch from a98e01b to 648c4b4 Compare October 14, 2021 07:29
Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this @ciampo!

@ciampo ciampo merged commit 24d603e into trunk Oct 14, 2021
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Oct 14, 2021
@ciampo ciampo deleted the refactor/components-toggle-group-control-option-button branch October 14, 2021 12:05
@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
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality [Type] Developer Documentation Documentation for developers
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants