feat: further simplify icon markup in t-shirt size component

[jianliao]

Description

The targets that I want to achieve include:

1. #753 - icon should allow resizing with a css variable.
2. Further simplify icon markup in T-shirt size component requested by this comment

Implementation detail:

1. I followed the article mentioned by @Westbrook to build a css variable cascading(fallback) chain. It looks like:

block-size: var(--spectrum-icon-user-defined-height, var(--spectrum-icon-tshirt-size-height, var(--spectrum-icon-default-height, var(--spectrum-alias-workflow-icon-size-m)))) 

inline-size: var(--spectrum-icon-user-defined-width, var(--spectrum-icon-tshirt-size-width, var(--spectrum-icon-default-width, var(--spectrum-alias-workflow-icon-size-m))))

• --spectrum-icon-user-defined-height is for component team(spectrum-react, spectrum-webcomponents) to customize the height.
• --spectrum-icon-tshirt-size-height will be provided by each t-shirt size component in its size class like .spectrum-ActionButton--sizeL. See here.
• --spectrum-icon-default-height. It is the default value when no user-defined and t-shirt size variable provided. For workflow icon height, it is default as DNA variable --spectrum-alias-workflow-icon-size-m.

2. To remove the class like spectrum-Icon--sizeS and spectrum-UIIcon-CornerTriangle100 from workflow icon and ui icon svg element, we need to provide the t-shirt size variable in its wrapper element CSS class like .spectrum-ActionButton--sizeM. See here. The html markup for an ActionButton would look like:

<button class="spectrum-ActionButton spectrum-ActionButton--sizeM">
  <svg class="spectrum-UIIcon spectrum-ActionButton-hold" focusable="false" aria-hidden="true">
    <use xlink:href="#spectrum-css-icon-CornerTriangle100" />
  </svg>
  <svg class="spectrum-Icon" focusable="false" aria-hidden="true" aria-label="Edit">
    <use xlink:href="#spectrum-icon-18-Edit" />
  </svg>
</button>

Open issues

1. To distinguish workflow icon and ui icon, I need to use spectrum-UIIcon for ui icon. This is not a new class, but I could not find anywhere we use it. It would be a breaking change.

2. I defined a series of CSS variables that are not from DNA. Not sure where to put them, I just insert them into components/vars/css/components/spectrum-icon.css. It is not ideal as this file will be overridden each time we upgrade the DNA.

3. In each component css, we have to define t-shirt size variable unused anywhere in that file. I have to comment out postcss-dropunusedvars for this purpose. Need to enhance this postcss plugin to provide a white list maybe?

4. All the new variables name are TBD. For example:

  --spectrum-icon-user-defined-height: var(--spectrum-icon-tshirt-size-height);
  --spectrum-icon-user-defined-width: var(--spectrum-icon-tshirt-size-width);
  --spectrum-icon-tshirt-size-height: var(--spectrum-icon-default-height);
  --spectrum-icon-tshirt-size-width: var(--spectrum-icon-default-width);
  --spectrum-icon-default-height: var(--spectrum-alias-workflow-icon-size-m);
  --spectrum-icon-default-width: var(--spectrum-alias-workflow-icon-size-m);

  --spectrum-ui-icon-user-defined-height: var(--spectrum-ui-icon-tshirt-size-height);
  --spectrum-ui-icon-user-defined-width: var(--spectrum-ui-icon-tshirt-size-width);
  --spectrum-ui-icon-tshirt-size-height: var(--spectrum-ui-icon-default-height);
  --spectrum-ui-icon-tshirt-size-width: var(--spectrum-ui-icon-default-width);
  --spectrum-ui-icon-default-height: var(--spectrum-alias-workflow-icon-size-m);
  --spectrum-ui-icon-default-width: var(--spectrum-alias-workflow-icon-size-m);

How and where has this been tested?

• How this was tested:
• Browser(s) and OS(s) this was tested with:

Screenshots

To-do list

• [ ] If my change impacts other components, I have tested to make sure they don't break.
• [ ] If my change impacts documentation, I have updated the documentation accordingly.
• [ ] I have read the CONTRIBUTING document.

• [ ] This pull request is ready to merge.

[jianliao]

@lazd, could you please review it again? So here is change:

1. Based on the what I learned from CSS variable spec, the dynamic cascade variables chain could not be defined on .spectrum scope because it is like root of all elements. Once a variable got evaluated, it could not be changed even in an inner/specific scope. We could only leave static global variables there.

2. I moved the cascade variable chain to components/vars/dynamic/*.css, variables defined there are all only could be figured out at runtime.

  --spectrum-icon-user-defined-height: var(--spectrum-icon-tshirt-size-height);
  --spectrum-icon-user-defined-width: var(--spectrum-icon-tshirt-size-width);
  --spectrum-icon-tshirt-size-height: var(--spectrum-alias-workflow-icon-size-m);
  --spectrum-icon-tshirt-size-width: var(--spectrum-alias-workflow-icon-size-m);

--spectrum-icon-user-defined-xxxx is for component team to defined. --spectrum-icon-tshirt-size-xxxx will be defined by component's t-shirt size specific class like .spectrum-ActionButton--sizeM.

3. Build script changed for cheating the bakeVar compile function to believe that above tokens in # 2 are existing at compile time.

[adobe-spectrum-bot]

Spectrum CSS documentation build successfully! :tada:

View the documentation

[adobe-spectrum-bot]

VRT successfully! :confetti_ball:

View the VRT result

[Westbrook]

Will these icon specific size classes be required after this change? https://github.com/adobe/spectrum-css/blob/tshirt-size-workflow-icon-continue/components/icon/workflow-icons.css#L21-L65

[jianliao]

No more specific size classes required.

[Westbrook]

I think I misstated my question...

Good to hear that is wouldn't be required!

Would Spectrum CSS still maintain those classes for optional use, going forward?

[lazd]

@Westbrook yes absolutely; we do want all of the standard icon sizes to be usable. This PR simply changes their sizes contextually where we know what size the icons need to be.

We still need to really chew on this and understand if we're being consistent if we go this route; take a look at ProgressBar, it has t-shirt sizing and uses FieldLabel, which also has t-shirt sizing. You must specify the size of the FieldLabel used within the ProgressBar manually. It would be overreach for ProgressBar to change FieldLabel's variables for font size, weight, margin, etc contextually.

Given the ProgressBar and FieldLabel issue, does it make sense for Button to reach into Icon and do the same thing? Yes, it's only dimensions, but where do we draw the line?

[Westbrook]

I'd less call this "reaching into" a component than defining what scope a component is delivered in, which would lead me to think that a ProgressBar effecting a FieldLabel internal to it would be little different than a Button effecting the Icon within it.

In either case, would delivering the internal component at a different size than the external element be considered "Spectrum correct"? If not, I'd hope that the cascade managed this without need of specific classes, this goes the extra mile of reducing the likelihood of breaking changes like we've seen in the move from sizeS to sizeM as far as Icons in Buttons, recently. In the end, I'm predominately looking for ways to better prevent these sorts of changes making their way down stream to my consumers. Then, the use of classes to make a purposeful departure from the design spec would still be possible as a specific opt-in, and would make sense why the classes I referenced above would continue to exist.

[lazd]

I'd less call this "reaching into" a component than defining what scope a component is delivered in, which would lead me to think that a ProgressBar effecting a FieldLabel internal to it would be little different than a Button effecting the Icon within it.

I can definitely see that, but where do we draw the line? Only icons can be contextually sized? I think that's fair, honestly, but need to hear from the rest of the team.

In either case, would delivering the internal component at a different size than the external element be considered "Spectrum correct"?

Almost certainly not, which is definitely a good argument for this approach.

If not, I'd hope that the cascade managed this without need of specific classes, this goes the extra mile of reducing the likelihood of breaking changes like we've seen in the move from sizeS to sizeM as far as Icons in Buttons, recently.

My point exactly on the other PR comment https://github.com/adobe/spectrum-web-components/issues/669#issuecomment-736663089; token-driven icon sizing means that consumers aren't on the hook for updating markup when a design decision is made. That said, the sizeS to sizeM change was a LOOONNGGG time coming, we had legacy code from 3 years ago in there for icon sizing, and we finally unified it. I absolutely don't see that happening again any time soon, and if it did, it would be an update to the size of a medium icon, not a change in the scale (i.e. small is renamed to medium).

In the end, I'm predominately looking for ways to better prevent these sorts of changes making their way down stream to my consumers. Then, the use of classes to make a purposeful departure from the design spec would still be possible as a specific opt-in, and would make sense why the classes I referenced above would continue to exist.

Agreed. I think we're on the right track, but let's be sure to think about the system as a whole when we make changes that introduce a completely new API, as this one does (variable driven icon sizing is API!).

[lazd]

@jianliao please rebase 🙏

[adobe-spectrum-bot]

VRT successfully! :confetti_ball:

View the VRT result

[adobe-spectrum-bot]

Spectrum CSS documentation build successfully! :tada:

View the documentation

[adixon-adobe]

One thing I'll add to the FieldLabel -> ProgressBar discussion here is that I've been thinking of "specifically-sized" sizing, and "auto-sized based on context" sizing. So ProgressBar auto-sizing to the right size for FieldLabel really does make sense to me (as long as the interface for doing so is well encapsulated).

CSS variables seem to give the tools to do this, but it might be overly cumbersome to support directly in SpectrumCSS in some contexts. Icons are simple enough that I think it does.

[castastrophe]

@pfulton As we're reworking the tooling elsewhere and seem to have a new approach to t-shirt size variables, should this PR be closed? Is there anything else in here we want to maintain?