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

ui: Native CSS Icon Composition #12461

Merged
merged 21 commits into from
Mar 15, 2022
Merged

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Feb 28, 2022

It's a biggie.

TLDR;

From a review standpoint you are probably best looking at https://github.com/hashicorp/consul/tree/413b0b9785247450e38942e059836e63a876d58b/ui/packages/consul-ui/app/styles/base/icons specifically:

Viewing individual commits will also probably be easier. Please ask if you have any questions.


We perhaps infamously have always used CSS for all our icons in the Consul UI, and perhaps even more infamously used %placeholder/@extends in order to really easily add icons to HTML elements from The Outside.

This PR begins to wean ourselves off of using a CSS processor for our icons. i.e. this PR sets us up for complete removal of %placeholder/@extends for our icon support, therefore only using native browser features for our decorative iconography.

Probably the best place to continue reading are the docs themselves

Also Preview Site. We've also reverted this

Importantly we moved our icon sizing from ems + font-size to rems, which I read somewhere we should be doing, although I'm not super convinced its the best approach. We only really needed to do this in one or two places, so if I need to revert back at some point then it's not a huge deal.

From #12222

Whilst this reverts this change of approach, the approach here reverts and adds the option of using font based sizing or absolute based styling using --icon-size: icon-000; (font based, or size=none/000) or --icon-size: icon-300; (absolute based: 16px/1rem)

What %placeholder/@extend gave/gives us

Ability to compose CSS multiple properties

@extend, @apply, composes: etc. all pretty much achieve the same thing. They allow you to group up collections of CSS declarations (custom or not) and then assign them to specific selectors. This allows a very high level of reuse of collections of CSS declarations whilst keeping a good clear boundary between the HTML and CSS ('what it is' vs 'what it looks like') in that you avoid littering your HTML with ever long lists of classes. For example, we currently have %frame placeholders which works like so:

%frame-red-500 {
  background-color: rgb(var(--tone-red-050));
  border-color: rgb(var(--tone-red-500));
  color: rgb(var(--tone-red-800));
}

You can easily give a div this 'frame' from within CSS-land with a one liner:

div {
  @extend %frame-red-500;
}

Selector Tagging/naming

Say you have a complicated selector, or a selector that you want to specifically expose to an end consumer to be styled how they like, using %placeholder/@extend you can tag specific important DOM nodes for easy styling purposes:

.component-name > div > header {
  @extend %component-name-header;
}
/* use */
%component-name-header {
  border: 10px red solid;
}

This is almost like the inverse of using placeholders to compose properties, here you are using placeholders as an identifier to allow you to easily define properties from The Outside. Importantly, the author of the component is free to change the DOM selector without having to change how the consumer refers to the DOM node.

.component-name > div > header {
  @extend %component-name-header;
}
/* use */
%component-name-header {
  border: 10px red solid;
}

I added an extra div there, but the consumer continues to use %component-name-header. This is all without using potentially problematic classNames.

CSS tree-shaking

Very importantly you should only ever use @extend along with %placeholders, if you start @extending normal class names things will go boom very quickly. Apart from that, %placeholders give you the additional characteristic that if you don't use them, they are excluded/'tree-shaken' out of the CSS build.

Linting/'type' checking

@extend will complain if the thing you are @extending doesn't exist, therefore as an engineer you get a build time warning if you try to use an icon that doesn't exist. This will give you an extra headstart when things go wrong.


So why are we moving away from '%placeholders/@extends'?

We currently use '%placeholders/@extends' in the Consul UI to great effect, at a rough guess I would say we use it in the high 100s of times and have had zero problems with that. It's been an incredibly useful tool for moving between different CSS approaches (bulma at al) and various refactors done over the lifetime of the v2 UI.

But, ideally I'd like to get to a place where we use no (or very little) CSS processing at all. As of right now, apart from %placeholder/@extends, we use no processor-only features. Almost everything is using standard CSS (we have a little nesting but that is easy to remove).

Moreover we are moving to use a more modern web platform featured codebase overall. This means being able to use CSS-in-JS (importantly not write CSS-in-JS). We need to be able to consume all of our 100s of CSS files separately for individual components which pretty much makes any CSS pre-processing out of the question, especially processing with SASS (we could still process using Javascript at runtime). I realized we'd got to a point where using various different techniques we can remove our reliance on '%placeholders/@extends' for CSS composition entirely and move to a completely native Web Platform based approach.

The replacement(s)

Constructable Stylesheets

Importantly, Constructable Stylesheets are only available on ShadowDOM nodes and documents, and whilst not 100% supported across all our browser targets there is a perfectly good fallback for browsers that don't yet support them and a good library for abstracting that detail away from the engineer. Constructable Stylesheets are great for composing CSS rules this means that Constructable Stylesheets have the selectors grouped with them, you then compose the rules not the properties/declarations. Whilst this helps us to a great extent this means you cannot apply a set of CSS properties/declarations to a DOM node if you need to select it differently. So overall it incredibly useful, but not the total package.

CSS Parts

CSS parts give us the ability to select/tag DOM nodes and expose them to end consumers with a reliable name/selector that then allows us to change the DOM without affecting the end user, essentially the 'Selector Tagging/naming' usecase from above. We can now change this:

.component-name > div > header {
  @extend %component-name-header;
}
/* use */
%component-name-header {
  border: 10px red solid;
}

into

<!-- within some ShadowDOM -->
<header part="header">
</header>
/* use */
component-name::part(header) {
  border: 10px red solid;
}

Again, this is only available to us when using ShadowDOM.

CSS Variables

We recently almost entirely moved away from SASS variables in our application (our external iris package has been based on CSS properties since day one which was converted to SASS as a build step). CSS properties are much more useful than SASS variables as they are runtime, but do have the double edged sword of them also cascading.

As well as being used to specify single properties, additionally for some use cases CSS properties can be used for enabling multiple CSS properties/declarations using the conditional space hack/trick.

CSS Keyframes

It turns out we've had a partial solution to composing groups of CSS declarations in native CSS for quite sometime:

@keyframes frame-red-500 {
  100% {
      background-color: rgb(var(--tone-red-050));
      border-color: rgb(var(--tone-red-500));
      color: rgb(var(--tone-red-800));
  }
}
div {
  animation-name: frame-red-500;
  animation-play-state: paused; /* doesn't seem to be required, but just in case */
  animation-fill-mode: forwards; /* stick the animation to 100% */
}

Sets of related compositions can be grouped together and applied by sending the timeline a specific point:

@keyframes frame-red {
  50% {
      background-color: rgb(var(--tone-red-050));
      border-color: rgb(var(--tone-red-300));
      color: rgb(var(--tone-red-500));
  }
  100% {
      background-color: rgb(var(--tone-red-050));
      border-color: rgb(var(--tone-red-500));
      color: rgb(var(--tone-red-800));
  }
}
div {
  animation-name: frame-red;
  animation-play-state: paused; /* doesn't seem to be required, but just in case */
  animation-fill-mode: forwards; /* stick the animation */
  animation-iteration-count: 0.5; /* go to 50%*/
}

Turns out this last approach works wonderfully with our CSS icons. We can name the icons, provide aliasing, sizing, even icon variations similar to font-variations. All provided by native Web Platform CSS.

Tree-shaking vs explicit import/inclusion.

Probably the biggest thing to note is that as we are including all icons in our built output our CSS has ballooned to around 192kb gzipped and this is currently very noticeable when viewing the preview site. This is because we are no longer using the tree-shaking properties of %placeholder. Which means ~1MB worth of iconography is being downloaded by our users.

The approach here is to only import what we use, i.e. explicit importing of individual icons.

Between now and our next major release we need to catalog all the icons we currently use and then just stop to including/importing all the icons we don't need instead of including everything and tree-shaking it out.

You'll see in the PR what we've split out all our icons into 100s of separate files, this means we can very granularly include what we want and leave the rest.

'Type checking'/Linting

Whilst not available in all our browser targets as yet, CSS.registerProperty is a thing coming soon, and handily is available in the browser we use most for building the UI. This means that we can use runInDebug to use CSS.registerProperty to add dev-time only enums/types for any CSS custom properties we add. Using this characteristic we can provide a warning to let us know when we need to import a new 'icon CSS bundle', i.e. if --icon-name: icon-brand-new; is not a valid value, we'll get a console error and know we need to import new CSS for the icon. Once its more widely available we can remove the runInDebug and take advantage of runtime type/enum checking for all our icons/CSS. I'm thinking I can use this initially to help me with the cataloging of icons. Hopefully I can register --icon-name, and slowly add the names that we are using as I see the errors in console. Once I'm done there I'll have a full list of all the icons we currently use to use for configuring what we include.

Lastly, again please make sure you read the docs themselves to see the approach, this description is mainly The Why and The How, not so much The What.

Things I'm not 100% happy about

This PR has almost 3000 files changed, they are 99.99% machine generated and shouldn't really be in this main repository. Everything is currently pointing towards an externally published package of some sort, something which I'm still thinking about. I'd be love to hear any thoughts/ideas/opinions on that. I don't think its a big deal that I'm adding all these files here temporarily.

@github-actions github-actions bot added the theme/ui Anything related to the UI label Feb 28, 2022
@vercel vercel bot temporarily deployed to Preview – consul February 28, 2022 17:59 Inactive
@johncowen johncowen marked this pull request as draft February 28, 2022 18:05
@johncowen johncowen requested review from a user, natmegs, amyrlam and jgwhite March 1, 2022 12:29
@johncowen johncowen marked this pull request as ready for review March 1, 2022 12:29
@vercel vercel bot temporarily deployed to Preview – consul March 1, 2022 12:52 Inactive
@vercel vercel bot temporarily deployed to Preview – consul March 1, 2022 12:52 Inactive
@vercel vercel bot temporarily deployed to Preview – consul March 1, 2022 13:44 Inactive
@vercel vercel bot temporarily deployed to Preview – consul March 7, 2022 11:12 Inactive
@johncowen johncowen force-pushed the ui/feature/native-css-icon-composition branch from 8a07d5a to ac38332 Compare March 7, 2022 12:03
@vercel vercel bot temporarily deployed to Preview – consul March 7, 2022 12:03 Inactive
@vercel vercel bot temporarily deployed to Preview – consul March 7, 2022 15:54 Inactive
@johncowen
Copy link
Contributor Author

Almost forgot but wanted to note here that in amongst the commits after writing the original description we went through all the icons we were actually using and only included those by commenting all the imports out and then uncommenting the ones we were using. This got us down to ~60kb CSS again (gzipped) which I'm still pretty happy with considering its got all our icons in it also.

Although thats nowhere near the 192kb gzipped for all the icons as mentioned in the description above, it is (I think it was around) 5-6kb bigger than it was originally. It was originally around 56kb gzipped or something as far as I remember (I might come back here and add a more factual before/after).

I figured this is because we are including both resolutions of icon whether you use both or not. Solution to this is just to even more finely-grain the imports, which should just be a little more fine tuning of the svg2icon generation script in iris.

For the sake of 5-6kb and time what we have on right now, I'm happy to punt this saving for another time.

Little note, I'm letting this sit just a little while longer whilst I get a little more feedback on the approach, and make sure I've had time to take the blinkers off 😎 . I've rebased with main and PRed another PR on top here that was waiting on approval of this one so we can keep that moving.

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – consul March 15, 2022 12:40 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging March 15, 2022 12:40 Inactive
@johncowen johncowen merged commit 5ab7e48 into main Mar 15, 2022
@johncowen johncowen deleted the ui/feature/native-css-icon-composition branch March 15, 2022 12:54
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/606744.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/ui Anything related to the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants