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

Rename "Dashicon" component to "Icon" to enable a generic icon interface #10502

Closed
jasmussen opened this issue Oct 11, 2018 · 10 comments
Closed
Labels
[Type] Enhancement A suggestion for improvement.
Milestone

Comments

@jasmussen
Copy link
Contributor

A user in the WordPress #core-editor chat asked for one of the new Gutenberg icons to be exposed for usage in their plugin. We can't yet do that because it's not a Dashicon and doesn't fit with the grid.

Already now, Gutenberg uses a mixture of Dashicons from the icon-font, Dashicons from the SVG component, Material Design icons for the block library, and a few custom icons for things like the drag handle. This could definitely be streamlined.

The Material icons were introduced into the block library in effort to increase the pool of available icons in Dashicons, so we can help avoid a situation where two different blocks use the same icon, causing confusion like when the Cover Image used the same icon as the Image block.

Although having a custom icon set for WordPress is nice for branding, right now that process is a bottleneck to block developers because it relies on icons having to be added into that set before users can gain access to them. This is not helpful to ensuring a diverse set of icons for plugin developers to use in their blocks.

Long term, WordPress need icons that:

  • there are a lot of, ideally an ever-growing set
  • embrace the open source nature of WordPress
  • are consistent across WordPress and even across plugins as well
  • are visually consistent, yet gives us the opportunity to add a bit of our own branding in the process
  • are easy to apply across our products, for native apps and across the web in SVG and React environments

Dashicons (per efforts outlined here) can play a part in ensuring the WordPress branding. Material icons can ensure an expanded set beyond that.

Gutenberg can start to embrace this future today, by renaming the <Dashicon> component to something more generic, like <Icon>.

Nothing else would have to change at the moment, but in future releases, we would be able to expand this component to include a "set" parameter to add additional icons from, for example, Material icons, perhaps even provide cross-icon mapping. Ideally we'd do this name change before the 5.0 merge, so plugins can start future proofing their icon includes even today.

@jasmussen jasmussen added the [Type] Enhancement A suggestion for improvement. label Oct 11, 2018
@jasmussen jasmussen added this to the WordPress 5.0 milestone Oct 11, 2018
@sgomes
Copy link
Contributor

sgomes commented Oct 11, 2018

Thank you, @jasmussen!

I think this is a great plan, as moving beyond a hardcoded icon set is interesting both from a conceptual and a technical point of view. Being technical-minded, I'll focus on the latter, but I'm sure other folks can weigh in on the former :) Do let me know if you'd prefer for me to move the technical discussion to a separate issue, though!

So from a technical point of view, I'd like to see this change be accompanied by a rethink of the approach being used. At the moment, we're storing the icon data as a large switch statement that resolves to a string containing the path data for the chosen icon.

This approach has several issues:

  • It negatively affects the bundle size, adding ~30KB to the components bundle. Byte for byte, JS is the most expensive resource for a browser to handle, as it comes with parsing and compilation costs attached. And it's not even replacing the runtime image processing costs; just adding to them.
  • It reduces cacheability for these icons. Every time a new Gutenberg release comes out with a new components bundle, the icon data will be invalidated in users' browsers along with the application code.
  • It removes potential optimisation paths for images on the browser. Since there's no resource URL or common HTMLImageElement instance, each instance of an icon is effectively a different image, and the browser won't be able to e.g. make use of in-memory caches for the rasterised versions of the SVGs.
  • It introduces a maintenance burden, by having an intermediate code generation step in the pipeline, which in the case of dashicons seems to live outside the regular build pipeline and require developer intervention.

There are several alternative approaches to consider, each with pros and cons:

  • Using an icon font. While this was once a good approach, the downsides likely outweigh the benefits nowadays.
    • Pros:
      • Keeps the icons in a format that's easy for the browser to work with
      • Easily stylable
    • Cons:
      • Accessibility issues
      • Cacheability isn't very granular
      • Rendering isn't pixel-perfect and can be somewhat inconsistent between browsers
      • The fallbacks aren't great
      • Hard to control prioritisation / loading strategy
      • Not the most optimal representation for vectorial data, from a file size perspective
      • Requires a build-time generation step
  • Using plain SVGs in an <img> tag. Simplest and most scalable approach, but doesn't work very well if styling is important.
    • Pros:
      • Requires no build-time work
      • Consistent rendering
      • Keeps the icons in the best format for the browser
      • Great cacheability with per-icon granularity
    • Cons:
      • Not easily stylable; would require CSS filters or similar. This likely invalidates the approach.
      • The large number of resources could be a problem for HTTP 1.1 connections
      • If most icons are being used, it could end up being more bandwidth-intensive than needed, since individual resources don't gzip as well as a single, concatenated one
  • Using SVG sprites with xlink:href. Interesting compromise between the two approaches above.
    • Pros:
      • Consistent rendering
      • Keeps the icons in the best format for the browser
      • Easily stylable
    • Cons:
      • Cacheability isn't very granular
      • Requires a build-time generation step
      • Would require less-than-optimal workarounds for old IE (not entirely sure which version; would require testing). One possibility would be a different code path that AJAX loads the sprite file and inlines the SVG directly into the page, which is essentially the same approach as we now have, but with the image data living in an external resource instead.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Oct 11, 2018

I continue to be skeptical about the embrace of Material Design Icons, and would push back on the idea that 3rd party blocks and core blocks should ever use the same icon set.

To me, a much better approach from the start would have been to expand the set of dashicons available to cover all the new blocks and Gutenberg UI elements, and make it clear that third parties should avoid use of the core icon set and instead find third party icon sets that fit the requirements.

That would have ensured Core Block icons have a consistent, familiar style but also are visually distinct from third party blocks. It would give users clarity on which blocks are trusted and "official" and which are provided by third parties. Because of the difficulties around extending core blocks, I'm already seeing cases where developers are building custom blocks that are essentially duplicates of core blocks with maybe one or two additional/tweaked options. (Heck, I've had to do it myself.)

Expanding the set of icons doesn't make a difference in those cases—there are only so many material design icons to represent "image" or "heading" or "columns". The only thing that would make a difference is giving core icons a different icon set entirely, and strongly and publicly discouraging users from using that set in their blocks.


I'm just not convinced it is good for core to be providing any type of icon interface at all. This leads directly to code bloat (loading in a full set of icons that may or may not be used), potential issues with maintainability and consistency, etc. I strongly feel the preferred mechanism for adding icons should continue to be inline SVG. (Clever developers will also realise that they could add one of the many material design icon components as a dependency and use that, if they're really opposed to pasting SVG code.)





All that said, I realise that feedback is a day late and a dollar short, not particularly actionable, and largely subjective. I recognise that developers don't always behave the way we hope they do, and that has to be factored in as well.

@jasmussen
Copy link
Contributor Author

Good thoughts Chris, I'll respond more in-depth later, but just as a quick note: this effort is intended only to make it easier for plugin developers to choose icons, including from Dashicons, if they are unable to provide their own, which should also remain an option forever. And if indeed the plugin developer wants to provide their own set entirely, I don't see there's any reason to block that.

But just in order to provide a consistent baseline, a good set of guidelines, and more importantly work in a performant way including in React Native environments, it makes sense to look at a generic interface for adding those icons.

@chrisvanpatten
Copy link
Member

Thanks @jasmussen. I totally appreciate there are a lot of factors at play so just to be clear I'm very much coming from a place of 'strong opinions, loosely held' 😅

@mtias mtias modified the milestones: WordPress 5.0 RC, Future: 5.1 Nov 16, 2018
@gziolo
Copy link
Member

gziolo commented Apr 24, 2019

@jasmussen how this issue relates to #9647? Can we consolidate them?

@jasmussen
Copy link
Contributor Author

It relates tangentially in that one could happen without the other. But your could create a new meta ticket for it if you'd like to simplify.

We need a simple icon component that isn't tied to a specific set, that's easy for developers to use and for us to maintain, and easy to expand with for example material icons in the future.

@sgomes
Copy link
Contributor

sgomes commented Apr 24, 2019

FYI, Material Icons in Calypso were recently added using an SVG External Content approach, with svg4everyone as a polyfill for old browsers: Automattic/wp-calypso#32364

This approach works well, as it's a good compromise between cacheability and number of HTTP requests, and avoids large amounts of procedurally-generated JSX. It also helps keep images as images (instead of JS), which is good for the browser to be able to prioritise requests and reduce overall CPU usage.

I'm currently looking into applying the same approach to gridicons and social-logos in Calypso.

@paaljoachim
Copy link
Contributor

Btw this issue: Add Icons Block could use some feedback. #16484

@jasmussen
Copy link
Contributor Author

@youknowriad The Icon component you've been working on seems to fix this ticket. When do you think it would be appropriate to close this one?

@youknowriad
Copy link
Contributor

I think we can close this issue now, what remains now is more refactorings to existing usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

7 participants