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] Use internal Icon component for ToggleGroupControlOptionIcon #40717

Merged
merged 2 commits into from
May 2, 2022
Merged

[components] Use internal Icon component for ToggleGroupControlOptionIcon #40717

merged 2 commits into from
May 2, 2022

Conversation

danieliser
Copy link
Contributor

@danieliser danieliser commented Apr 29, 2022

What?

Trying to use dashicon style string based icon props for the <ToggleGroupControlOptionIcon> component would throw errors expecting a component and getting a string.

This fix corrects that by switching to the more versatile <Icon> from @wordpress/componenets

Why?

<ToggleGroupControlOptionIcon> was using the SVG based <Icon> component from @wordpress/icons which does not support dashicon based strings.

How?

Switched from importing Icon from @wordpress/icons to importing it as internal from within @wordpress/components.

Testing Instructions

  1. Try using the new component with a dashicon.
<ToggleGroupControlOptionIcon
	icon="hidden"
  1. You can confirm the fix by simply replacing the "string" prop with a JSX Icon
import { Icon } from '@wordpress/components';

<ToggleGroupControlOptionIcon
	icon={ <Icon icon="hidden" /> }

Screenshots or screencast

image

@danieliser danieliser requested a review from ajitbohra as a code owner April 29, 2022 07:43
@danieliser danieliser changed the title Use internal Icon component to support dashicons [components] Use internal Icon component to support dashicons Apr 29, 2022
@danieliser danieliser changed the title [components] Use internal Icon component to support dashicons [components] Use internal Icon component for ToggleGroupControlOptionIcon Apr 29, 2022
@ntsekouras ntsekouras requested review from mirka and ciampo April 29, 2022 11:50
@ntsekouras ntsekouras added the [Package] Components /packages/components label Apr 29, 2022
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Makes sense, thank you!

One small thing before we merge, could you add an entry in the components changelog so people can know about this nice enhancement?

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.

On top of what @mirka said, I also wanted to flag that you won't be able to use Dashicons in Storybook at the moment (see #33984) — furthermore, although Dashicons are not deprecated, their use is currently not encouraged (cc @youknowriad who may be able to give more details on this).

@youknowriad
Copy link
Contributor

Yes, the only reason the Dashicon component is not officially deprecated is because blocks using the "icon" property can't switch to a component easily.

@danieliser
Copy link
Contributor Author

@youknowriad - So should this even get merged? I assume the long term plan is to move all the dashicons into the icon library at some point?

Happy to add changelog note if still want me to.

@ciampo
Copy link
Contributor

ciampo commented May 2, 2022

In my opinion, the Icon component from the @wordpress/components package is a more comprehensive component than the one from @wordpress/icons package. It also seems reasonable that ToggleGroupControl uses the Icon component from the same package. Therefore, I think that we should merge the changes from this PR.

The Dashicons "deprecation" process seems complex and should be handled separately from this issue (plus, once we deprecated dashicons, we will have to make changes to the Icon component in @wordpress/components, and therefore it will affect ToggleGroupControl too).

@danieliser
Copy link
Contributor Author

Changelog note added.

Thanks for the feedback and quick reviews.

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Cool, thanks Daniel! 🚀

@mirka mirka merged commit dd1bef2 into WordPress:trunk May 2, 2022
@github-actions github-actions bot added this to the Gutenberg 13.2 milestone May 2, 2022
@priethor priethor added the [Type] Code Quality Issues or PRs that relate to code quality label May 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants