Update the Dashicon component to rely on the font that ships with WordPress #20003

youknowriad · 2020-02-03T11:36:18Z

With the new @wordpress/icons package, the Dashicon component usage should be deprecated but even if it's deprecated, its code will have to remain on the project for Backward compatibility reasons. This is not a problem for any random component but it's a problem for the Dashicon because that component is huge (it includes all the SVGs) and it can't be tree-shaken for third-party usage (I know for example that Woo are struggling with this).

In this PR, I update the implementation of the component to rely on the font and styles that already ship with WordPress. This has some advantages:

It ensures backward compatibility for third-party usage of this component in WordPress (as this is a hard rule for us).
The bundle size is a lot smaller, so even if the component is indirectly used with npm package usage (like in the Button component), it doesn't harm the consumer's bundle size.

This is a breaking change though for the npm package itself because it means that component is basically useless outside of WordPress but breaking changes in npm are ok.

Notes

If you test the PR, you'll notice that some dashicons are not appearing, that's because the component here and the Dashicons version that ships with WordPress are not in sync. To solve this, we need to update WordPress Core Dashicons font to the last version with all the latest icons.

This also can't be merged now, we need to refactor all the mobile usage of the component and rely on the icons package instead. cc @Tug @hypest

Thoughts?

nerrad

Generally, this makes sense and helps keep the bundle-size down. However for back-compat usage consuming code must ensure any styles list dashicons as a dependency for the style registration. This likely is already taken care of in most cases in the wp-admin, but outside of wp-admin it won't be the case.

Along with the CHANGELOG.md mention of the breaking change, this might be worth a dev-note for whatever version of WP this lands in as well?

Also, is there any potential risk of validation errors for usage of <DashIcon/> that might be saved to post content?

aduth · 2020-02-03T13:25:16Z

This is a breaking change though for the npm package itself because it means that component is basically useless outside of WordPress but breaking changes in npm are ok.

So for people who would want to use these icons outside of WordPress, is the suggested alternative that they use the @wordpress/icons package instead? Can that be included in the CHANGELOG?

However for back-compat usage consuming code must ensure any styles list dashicons as a dependency for the style registration.

Shouldn't we be making dashicons a dependency of the wp-components stylesheet to address this? Or are we hoping to avoid that?

youknowriad · 2020-02-03T13:36:47Z

This likely is already taken care of in most cases in the wp-admin, but outside of wp-admin it won't be the case.

When are blocks going to be used outsiide fo wp-admin, I mean even if they have frontend styles, I don't expect blocks to use Dashicon component in the frontend or can they?

youknowriad · 2020-02-03T13:37:52Z

Shouldn't we be making dashicons a dependency of the wp-components stylesheet to address this? Or are we hoping to avoid that?

I was hoping I can avoid that because it's true that it's possible but probably very rare. that said, we can still do it.

aduth · 2020-02-03T13:47:23Z

This likely is already taken care of in most cases in the wp-admin, but outside of wp-admin it won't be the case.

When are blocks going to be used outsiide fo wp-admin, I mean even if they have frontend styles, I don't expect blocks to use Dashicon component in the frontend or can they?

When we're saying "taken care of in wp-admin", are we talking about assuming the Dashicons font will already have been loaded by relying on it being some dependency of another stylesheet?

Tug · 2020-02-04T10:14:53Z

Thanks for bringing us in the loop!

This also can't be merged now, we need to refactor all the mobile usage of the component and rely on the icons package instead.

Ok 👍Should we create the branch, or do you plan to tackle it?

youknowriad · 2020-02-04T10:22:00Z

@Tug I think it could be done in small PRs refactoring the existing usage step by step. Feel free to tackle some of these.

Example PRs:

#19838
#19834
#19808

nerrad · 2020-02-04T12:44:13Z

When are blocks going to be used outsiide fo wp-admin, I mean even if they have frontend styles, I don't expect blocks to use Dashicon component in the frontend or can they

For me it's less about blocks being used more outside the admin and more about the Dashicons component being used outside the admin (as a part of a frontend client side rendering). We could treat this as similar to "package" use but if dashicons style is already a dependency on the wp-components style then at least in the WordPress context there should be no apparent breakage.

When we're saying "taken care of in wp-admin", are we talking about assuming the Dashicons font will already have been loaded by relying on it being some dependency of another stylesheet?

Ya, it's a dependency of a number of styles loaded in the wp-admin context so I'm not aware of (without completely verifying explicitly) of any wp-admin route that won't have the dashicons handle enqueued. With that said, like any dependencies, I don't think we should rely on that always being the case.

Even with my comments here, I'd still like to see this work done. My comments are just more of awareness of potential impact. So to summarize, should we go ahead with this change, these are the things from my perspective I think we'll want to cover:

It's a breaking change for the package (so should be communicated as such).
Make the WordPress core dashicons handle a dependency of wp-components.
Publish a devnote about the change to surface for developers that might have used the <DashIcons/> component in some custom react app in the WordPress environment but outside of the wp-admin context.

youknowriad · 2020-02-18T12:44:05Z

If you test the PR, you'll notice that some dashicons are not appearing, that's because the component here and the Dashicons version that ships with WordPress are not in sync. To solve this, we need to update WordPress Core Dashicons font to the last version with all the latest icons.

@ntwb Do you think you can help with this? Update the dashicon font in Core with all the icons including the ones created for Gutenberg?

jasmussen · 2020-02-18T13:13:04Z

Update the dashicon font in Core with all the icons including the ones created for Gutenberg?

It should be "enough" to move all the SVGs from the sources/svg/gutenberg into the sources/svg folder. Alternately the build script can be made to include the gutenberg subfolder when compiling the icon font, which it currently ignores.

github-actions · 2020-02-19T09:53:24Z

Size Change: -34.3 kB (2%)

Total Size: 1.17 MB

Filename	Size	Change
`build/annotations/index.js`	3.67 kB	+1 B
`build/block-directory/index.js`	8.64 kB	+120 B (1%)
`build/block-editor/index.js`	128 kB	-21 B (0%)
`build/block-library/index.js`	139 kB	+80 B (0%)
`build/block-serialization-default-parser/index.js`	1.88 kB	-1 B
`build/blocks/index.js`	47.8 kB	+2 B (0%)
`build/components/index.js`	167 kB	-34.5 kB (20%)	🎉
`build/components/style-rtl.css`	15.5 kB	-2 B (0%)
`build/components/style.css`	15.4 kB	-2 B (0%)
`build/compose/index.js`	9.67 kB	-10 B (0%)
`build/data-controls/index.js`	1.28 kB	+1 B
`build/data/index.js`	8.55 kB	+1 B
`build/deprecated/index.js`	771 B	-1 B
`build/dom/index.js`	4.47 kB	-2 B (0%)
`build/edit-post/index.js`	305 kB	+1 B
`build/edit-site/index.js`	19.1 kB	+44 B (0%)
`build/edit-widgets/index.js`	12.2 kB	+1 B
`build/editor/index.js`	45.3 kB	+2 B (0%)
`build/i18n/index.js`	3.56 kB	+1 B
`build/is-shallow-equal/index.js`	710 B	-1 B
`build/list-reusable-blocks/index.js`	3.12 kB	-1 B
`build/notices/index.js`	1.79 kB	-1 B
`build/plugins/index.js`	2.56 kB	-1 B
`build/redux-routine/index.js`	2.85 kB	-1 B
`build/rich-text/index.js`	13.9 kB	+1 B
`build/shortcode/index.js`	1.7 kB	+1 B
`build/url/index.js`	4.06 kB	+1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/api-fetch/index.js`	3.41 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/style-rtl.css`	943 B	0 B
`build/block-directory/style.css`	942 B	0 B
`build/block-editor/style-rtl.css`	11 kB	0 B
`build/block-editor/style.css`	11 kB	0 B
`build/block-library/editor-rtl.css`	8.69 kB	0 B
`build/block-library/editor.css`	8.69 kB	0 B
`build/block-library/style-rtl.css`	7.6 kB	0 B
`build/block-library/style.css`	7.59 kB	0 B
`build/block-library/theme-rtl.css`	741 B	0 B
`build/block-library/theme.css`	741 B	0 B
`build/block-serialization-spec-parser/index.js`	3.1 kB	0 B
`build/core-data/index.js`	12.2 kB	0 B
`build/date/index.js`	31.9 kB	0 B
`build/dom-ready/index.js`	568 B	0 B
`build/edit-navigation/index.js`	10.7 kB	0 B
`build/edit-navigation/style-rtl.css`	868 B	0 B
`build/edit-navigation/style.css`	871 B	0 B
`build/edit-post/style-rtl.css`	6.24 kB	0 B
`build/edit-post/style.css`	6.22 kB	0 B
`build/edit-site/style-rtl.css`	3.13 kB	0 B
`build/edit-site/style.css`	3.13 kB	0 B
`build/edit-widgets/style-rtl.css`	2.55 kB	0 B
`build/edit-widgets/style.css`	2.55 kB	0 B
`build/editor/editor-styles-rtl.css`	492 B	0 B
`build/editor/editor-styles.css`	493 B	0 B
`build/editor/style-rtl.css`	3.8 kB	0 B
`build/editor/style.css`	3.8 kB	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/index.js`	7.71 kB	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	621 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/media-utils/index.js`	5.31 kB	0 B
`build/nux/index.js`	3.4 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/primitives/index.js`	1.41 kB	0 B
`build/priority-queue/index.js`	789 B	0 B
`build/server-side-render/index.js`	2.77 kB	0 B
`build/token-list/index.js`	1.27 kB	0 B
`build/viewport/index.js`	1.85 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

Helps fix WordPress/gutenberg#20003. Back in the day when we moved from a manual process, to a build script for Dashicons, we put Gutenberg icons in a separate folder that was not included in the icon font. The context for that started in a meeting here: https://wordpress.slack.com/archives/C03EESJAW/p1498836928887730 — this meeting outlined the process of creating a trac ticket for every icon created and approved separately. Some of that process is outlined in https://wordpress.slack.com/archives/C03EESJAW/p1498838219357464. In order to stay nimble and allow us to move fast with creating Gutenberg icons, it was suggested (https://wordpress.slack.com/archives/C03EESJAW/p1498838530476355) to put Gutenberg icons in a separte folder that is not included in the icon font, landing only in the React component. But the icons have been there for a bit now, and in effort to fix #20003, these icons now need to graduate. Because of their time in the editor, it feels fair to review them all in a single ticket.

jasmussen · 2020-02-21T09:04:05Z

I created WordPress/dashicons#402 to graduate the Gutenberg icons.

Helps fix WordPress/gutenberg#20003. Back in the day when we moved from a manual process, to a build script for Dashicons, we put Gutenberg icons in a separate folder that was not included in the icon font. The context for that started in a meeting here: https://wordpress.slack.com/archives/C03EESJAW/p1498836928887730 — this meeting outlined the process of creating a trac ticket for every icon created and approved separately. Some of that process is outlined in https://wordpress.slack.com/archives/C03EESJAW/p1498838219357464. In order to stay nimble and allow us to move fast with creating Gutenberg icons, it was suggested (https://wordpress.slack.com/archives/C03EESJAW/p1498838530476355) to put Gutenberg icons in a separte folder that is not included in the icon font, landing only in the React component. But the icons have been there for a bit now, and in effort to fix #20003, these icons now need to graduate. Because of their time in the editor, it feels fair to review them all in a single ticket.

senadir · 2020-05-12T10:54:58Z

I can see it's still being imported in @wordpress/components/icon to account for it, will have to find a way in webpack to remove it.

youknowriad · 2020-07-29T10:52:08Z

Now that the Dashicon font is updated on Core and frozen, I think it's time to revisit this PR. It seems ready for me.

hypest · 2020-07-30T12:47:06Z

Now that the Dashicon font is updated on Core and frozen, I think it's time to revisit this PR. It seems ready for me.

👋 @youknowriad , I wonder if you could add some testing instructions to help verify what icons are expected to work off the font?

@Tug or @ceyhun , can you have a new look on this PR? I haven't looked deeper but, I wonder if the mobile app will also need to embed the font that holds the icons and break if the font is not there?

youknowriad · 2020-07-30T12:55:26Z

I wonder if you could add some testing instructions to help verify what icons are expected to work off the font?

Do you have any ideas on mind. What would this test look like?

can you have a new look on this PR? I haven't looked deeper but, I wonder if the mobile app will also need to embed the font that holds the icons and break if the font is not there?

At this point, I expect that our usage of Dashicons on Gutenberg Core to be pretty small. It's still not entirely replaced with SVGs but if you find things that are still relying on Dashicons we can replace them with SVGs to make it work cross platform.

ceyhun · 2020-08-25T14:53:17Z

Do you have any ideas on mind. What would this test look like?

I'm not sure if this is the optimal way but what I did was searching for <Dashicon and one of the places was here:

gutenberg/packages/components/src/icon/index.js

Lines 21 to 29 in 8053ebc

    
           if ( 'string' === typeof icon ) { 
        
           	return ( 
        
           		<Dashicon 
        
           			icon={ icon } 
        
           			size={ dashiconSize } 
        
           			{ ...additionalProps } 
        
           		/> 
        
           	); 
        
           }

Than I searched for <Icon in .native.js files and found a string prop, this crashed the app when upload of an image failed for Gallery block:

gutenberg/packages/block-library/src/gallery/gallery-image.native.js

Lines 230 to 233 in 7532a48

    
           <Icon 
        
           	icon={ 'warning' } 
        
           	{ ...style.uploadFailedIcon } 
        
           />

There were also other places where the icon prop was passed in from a parent but I didn't go deeper. Those could also be a potential problem.

…dPress

youknowriad · 2020-09-16T09:59:04Z

@ceyhun I replaced these with svg usage, I also replaced a few other places. Let me know if you find any place I missed where dashicons are still being used.

I also noticed that on master, folks are continuing to use Dashicons for new code, so I'd like to move with this forward in order to officially deprecate the component and encourage folks to use the SVGs instead.

youknowriad · 2020-09-21T14:45:07Z

Anyone to approve this :)

jasmussen

This tests well for me, let's get this in.

ceyhun · 2020-09-22T09:43:07Z

@ceyhun I replaced these with svg usage, I also replaced a few other places. Let me know if you find any place I missed where dashicons are still being used.

Thanks for finding and fixing the other places @youknowriad! There still could be other places we missed but it looks good for now.

tellthemachines · 2020-10-06T03:50:24Z

packages/components/CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Breaking Change

 - `NumberControl` no longer automatically transforms values when rendering `value` into a `<input />` HTML element.
+- `Dashicon` component no longer renders SVGs. If you rely on this component, make sure to load the dashicon font.


Shouldn't this have gone under Unreleased?

yep, mistake 😬

No worries, I bumped the major version in last week's release 😄

youknowriad added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Feb 3, 2020

youknowriad requested review from ajitbohra, chrisvanpatten, jorgefilipecosta and talldan as code owners February 3, 2020 11:36

youknowriad self-assigned this Feb 3, 2020

youknowriad requested review from senadir, nerrad, aduth and jasmussen February 3, 2020 11:36

nerrad reviewed Feb 3, 2020

View reviewed changes

youknowriad mentioned this pull request Feb 6, 2020

RangeControls: Improve UI + UX #19916

Merged

6 tasks

youknowriad mentioned this pull request Feb 18, 2020

Remove more Dashicon component usage #20283

Merged

youknowriad force-pushed the try/rely-on-dashicon-font-for-component branch from 3f23814 to 36568bb Compare February 19, 2020 09:44

jasmussen mentioned this pull request Feb 21, 2020

Try: Graduate all Gutenberg icons to the icon font. WordPress/dashicons#402

Merged

jasmussen closed this in WordPress/dashicons#402 Mar 18, 2020

youknowriad force-pushed the try/rely-on-dashicon-font-for-component branch from 7b7e2bf to 428b0a3 Compare July 29, 2020 09:36

youknowriad requested review from Soean, spacedmonkey and TimothyBJacobs as code owners July 29, 2020 10:10

youknowriad requested a review from ntwb as a code owner July 29, 2020 10:40

ocean90 mentioned this pull request Sep 5, 2020

Add missing businesswoman dashicon to block library #25072

Closed

6 tasks

youknowriad added 6 commits September 16, 2020 10:18

Update the Dashicon component to rely on the font that ships with Wor…

6ace52e

…dPress

Remove unused stylesheet import

7f3ebf1

Add changelog and dependency

e157dd6

Fix unit tests

70d603b

Fix end2end tests and linting

b89b8da

Replace more dashicons

32e3b8e

youknowriad force-pushed the try/rely-on-dashicon-font-for-component branch from 8053ebc to 32e3b8e Compare September 16, 2020 09:55

youknowriad requested a review from ellatrix as a code owner September 16, 2020 09:55

jasmussen approved these changes Sep 22, 2020

View reviewed changes

youknowriad merged commit 06d1740 into master Sep 22, 2020

youknowriad deleted the try/rely-on-dashicon-font-for-component branch September 22, 2020 09:14

github-actions bot added this to the Gutenberg 9.1 milestone Sep 22, 2020

This was referenced Sep 23, 2020

[Mobile] - MediaUpload - Remove dashicons and use icons instead #25576

Merged

MediaUpload - Remove dashicons and use icons instead wordpress-mobile/gutenberg-mobile#2660

Merged

tellthemachines reviewed Oct 6, 2020

View reviewed changes

bobbingwide mentioned this pull request Oct 12, 2024

Enable dynamic display of latest plugins count, total downloads and related charts from post meta data bobbingwide/wp-top12#27

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the Dashicon component to rely on the font that ships with WordPress #20003

Update the Dashicon component to rely on the font that ships with WordPress #20003

youknowriad commented Feb 3, 2020 •

edited

Loading

nerrad left a comment •

edited

Loading

aduth commented Feb 3, 2020

youknowriad commented Feb 3, 2020

youknowriad commented Feb 3, 2020

aduth commented Feb 3, 2020

Tug commented Feb 4, 2020

youknowriad commented Feb 4, 2020

nerrad commented Feb 4, 2020

youknowriad commented Feb 18, 2020

jasmussen commented Feb 18, 2020

github-actions bot commented Feb 19, 2020 •

edited

Loading

jasmussen commented Feb 21, 2020

senadir commented May 12, 2020

youknowriad commented Jul 29, 2020

hypest commented Jul 30, 2020

youknowriad commented Jul 30, 2020

ceyhun commented Aug 25, 2020 •

edited

Loading

youknowriad commented Sep 16, 2020

youknowriad commented Sep 21, 2020

jasmussen left a comment

ceyhun commented Sep 22, 2020

tellthemachines Oct 6, 2020

youknowriad Oct 12, 2020

tellthemachines Oct 12, 2020

Update the Dashicon component to rely on the font that ships with WordPress #20003

Update the Dashicon component to rely on the font that ships with WordPress #20003

Conversation

youknowriad commented Feb 3, 2020 • edited Loading

nerrad left a comment • edited Loading

Choose a reason for hiding this comment

aduth commented Feb 3, 2020

youknowriad commented Feb 3, 2020

youknowriad commented Feb 3, 2020

aduth commented Feb 3, 2020

Tug commented Feb 4, 2020

youknowriad commented Feb 4, 2020

nerrad commented Feb 4, 2020

youknowriad commented Feb 18, 2020

jasmussen commented Feb 18, 2020

github-actions bot commented Feb 19, 2020 • edited Loading

jasmussen commented Feb 21, 2020

senadir commented May 12, 2020

youknowriad commented Jul 29, 2020

hypest commented Jul 30, 2020

youknowriad commented Jul 30, 2020

ceyhun commented Aug 25, 2020 • edited Loading

youknowriad commented Sep 16, 2020

youknowriad commented Sep 21, 2020

jasmussen left a comment

Choose a reason for hiding this comment

ceyhun commented Sep 22, 2020

tellthemachines Oct 6, 2020

Choose a reason for hiding this comment

youknowriad Oct 12, 2020

Choose a reason for hiding this comment

tellthemachines Oct 12, 2020

Choose a reason for hiding this comment

youknowriad commented Feb 3, 2020 •

edited

Loading

nerrad left a comment •

edited

Loading

github-actions bot commented Feb 19, 2020 •

edited

Loading

ceyhun commented Aug 25, 2020 •

edited

Loading