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

jianliao · 2020-11-09T22:59:32Z

Description

The targets that I want to achieve include:

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

Implementation detail:

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.

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

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.
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.
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?
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.

BREAKING CHANGE: .spectrum-FieldButton has been removed, use .spectrum-ActionButton instead

will be overwritten and fixed when https://git.corp.adobe.com/Spectrum/spectrum-dna/pull/458 merged

BREAKING CHANGE: markup now requires spectrum-ActionButton where all uses of spectrum-FieldButton were

docs: add migration guide and sizing examples BREAKING CHANGE: .spectrum-ProgressBar--large renamed to .spectrum-ProgressBar--sizeM, .spectrum-ProgressBar--large renamed to .spectrum-ProgressBar--sizeS is required for previous BREAKING CHANGE: ProgressBar .is-warning renamed to .is-notice, .is-critical renamed to .is-negative

docs: add examples with custom widths

adobe-spectrum-bot · 2020-11-17T05:08:20Z

VRT successfully! 🎊

View the VRT result

…inue

Westbrook · 2020-11-24T01:26:12Z

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 · 2020-11-30T22:58:28Z

No more specific size classes required.

Westbrook · 2020-11-30T23:09:04Z

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 · 2020-12-01T16:22:03Z

@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 · 2020-12-01T17:22:50Z

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 · 2020-12-01T17:30:55Z

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 adobe/spectrum-web-components#669 (comment); 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 · 2020-12-01T19:09:27Z

@jianliao please rebase 🙏

adobe-spectrum-bot · 2020-12-02T01:30:22Z

VRT successfully! 🎊

View the VRT result

adobe-spectrum-bot · 2020-12-02T01:31:12Z

Spectrum CSS documentation build successfully! 🎉

View the documentation

Westbrook · 2020-12-14T15:19:38Z

components/icon/icons.css

+
+.spectrum-Icon {
+  &,
+  img,


FYI: Nesting the size of img and svg in the same rules as .spectrum-Icon will likely cause issue in code like:

spectrum-css/components/tooltip/index.css

Lines 137 to 149 in 731aeab

.spectrum-Tooltip-typeIcon {

margin-inline-start: calc(

var(--spectrum-tooltip-icon-margin-x) - var(--spectrum-tooltip-padding-x)

);

margin-inline-end: var(--spectrum-tooltip-icon-margin-x);

width: var(--spectrum-tooltip-icon-size);

height: var(--spectrum-tooltip-icon-size);

align-self: flex-start;

flex-shrink: 0;

/* Adjusts for weird misalignment */

margin-block-start: 1px;

}

Maybe the goal is to ALWAYS use CSS Custom Properties in this sort of context? In that case we'd need to have any such instance updated as part of this PR or prior to an actual release of these changes.

An alternative would be to not directly size the img and svg content and set them as:

block-size: 100%; inline-size: 100%;

Thoughts?

adixon-adobe · 2021-01-05T22:53:04Z

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 · 2023-01-03T15:46:52Z

@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?

lazd added 30 commits October 23, 2020 10:31

feat: upgrade to @adobe/spectrum-tokens@6.0.0-beta.26

a33f965

fix: make Picker work again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e94775f

feat: support FORCE=true environment variable to allow broken components

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

cbdcd56

fix: make Menu work again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

5fb9867

fix: make Textfield/Textarea work again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

3ae0b45

feat: upgrade to @adobe/spectrum-tokens@6.0.0-beta.27

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

edd3969

fix: make Tags build again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

330a591

fix: make Picker build again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

27f1292

fix: make Popover and friends build again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

248da24

fix: make Switch build again

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

2f43959

fix: make Checkbox build again

58d547d

fix: make Radio build again

3ec52f5

feat: replace all FieldButton with ActionButton

bd2d16a

BREAKING CHANGE: markup now requires spectrum-ActionButton where all uses of spectrum-FieldButton were

fix: make Search build again

96ec12e

fix: make SearchWithin build again

01540c1

fix: make AssetList build again

c3065fd

fix: make InputGroup build again

5906aeb

fix: make FieldLabel build again

053e02d

fix: make Stepper build again

5675fa3

docs: update docs for t-shirt sizing

95bc432

fix: allow customizing ProgressBar width, fixes #1064

283bbf6

docs: add examples with custom widths

fix: make Tabs build again

5f7d3b8

fix: make Sidenav build again

134f266

fix: make Slider build again

bab372f

fix: make Dial build again

61605c0

refactor: properly use variables for FieldLabel ahead of t-shirt sizing

a565812

fix: correct width of track for side label

9f50465

lazd added 7 commits November 17, 2020 08:45

fix: correct Textarea padding

91f8d8d

fix: correct SplitButton padding

7a22893

fix: add back missing Button focus-ring

0b327eb

docs: fix disabled SearchWithin example

5b7345f

docs: correct FieldLabel and Combobox usage in wrapping study

7fbf2b4

fix: make default Tags small

baffbe5

docs: correct usage of Divider medium

fe7cae4

GarthDB assigned GarthDB and lazd and unassigned GarthDB Nov 18, 2020

GarthDB added the size-3 M label Nov 18, 2020

Merge branch 'spectrum-tokens-26' into tshirt-size-workflow-icon-cont…

41675d0

…inue

lazd mentioned this pull request Dec 1, 2020

sp-icon size attribute should be deprecated adobe/spectrum-web-components#669

Closed

Base automatically changed from spectrum-tokens-26 to main December 1, 2020 19:03

Merge branch 'main' into tshirt-size-workflow-icon-continue

5814fa5

Westbrook reviewed Dec 14, 2020

View reviewed changes

castastrophe closed this Mar 7, 2023

castastrophe deleted the tshirt-size-workflow-icon-continue branch March 6, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jianliao commented Nov 9, 2020 •

edited

Loading

adobe-spectrum-bot commented Nov 17, 2020

Westbrook commented Nov 24, 2020

jianliao commented Nov 30, 2020

Westbrook commented Nov 30, 2020

lazd commented Dec 1, 2020

Westbrook commented Dec 1, 2020

lazd commented Dec 1, 2020 •

edited

Loading

lazd commented Dec 1, 2020

adobe-spectrum-bot commented Dec 2, 2020

adobe-spectrum-bot commented Dec 2, 2020

Westbrook Dec 14, 2020

adixon-adobe commented Jan 5, 2021

castastrophe commented Jan 3, 2023

	.spectrum-Tooltip-typeIcon {
	margin-inline-start: calc(
	var(--spectrum-tooltip-icon-margin-x) - var(--spectrum-tooltip-padding-x)
	);
	margin-inline-end: var(--spectrum-tooltip-icon-margin-x);
	width: var(--spectrum-tooltip-icon-size);
	height: var(--spectrum-tooltip-icon-size);
	align-self: flex-start;
	flex-shrink: 0;

	/* Adjusts for weird misalignment */
	margin-block-start: 1px;
	}

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

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

Conversation

jianliao commented Nov 9, 2020 • edited Loading

Description

The targets that I want to achieve include:

Implementation detail:

Open issues

How and where has this been tested?

Screenshots

To-do list

adobe-spectrum-bot commented Nov 17, 2020

Westbrook commented Nov 24, 2020

jianliao commented Nov 30, 2020

Westbrook commented Nov 30, 2020

lazd commented Dec 1, 2020

Westbrook commented Dec 1, 2020

lazd commented Dec 1, 2020 • edited Loading

lazd commented Dec 1, 2020

adobe-spectrum-bot commented Dec 2, 2020

adobe-spectrum-bot commented Dec 2, 2020

Westbrook Dec 14, 2020

Choose a reason for hiding this comment

adixon-adobe commented Jan 5, 2021

castastrophe commented Jan 3, 2023

jianliao commented Nov 9, 2020 •

edited

Loading

lazd commented Dec 1, 2020 •

edited

Loading