feat(fieldlabel)!: core-tokens implementation (CSS-102) #1476

bernhard-adobe · 2022-07-13T23:29:23Z

Description

BREAKING CHANGE: This migrates the Field label component to core tokens.

https://jira.corp.adobe.com/browse/CSS-102
Web Component Implementation: adobe/spectrum-web-components#2529 refactor(field-label): use core tokens #2529

How and where has this been tested?

How this was tested: comparing http://localhost:3002/docs/fieldlabel.html with the https://opensource.adobe.com/spectrum-css/fieldlabel.html component before and after.
Browser(s) and OS(s) this was tested with: Chrome Canary Version 105.0.5148.0 (Official Build) canary (arm64)

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.
I have tested these changes in Windows High Contrast mode.

This pull request is ready to merge.

netlify · 2022-07-13T23:32:07Z

✅ Deploy Preview for spectrum-css ready!

Name	Link
🔨 Latest commit	`e0cce17`
🔍 Latest deploy log	https://app.netlify.com/sites/spectrum-css/deploys/62cf573b5d3d4f0008aa7c35
😎 Deploy Preview	https://deploy-preview-1476--spectrum-css.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

components/fieldlabel/index.css

bernhard-adobe · 2022-07-13T23:35:14Z

components/fieldlabel/index.css

-  --spectrum-tableform-labelsabove-item-gap: var(--spectrum-global-dimension-size-100);
+  /* for /docs/form.html to set field-label inside form */
+  --spectrum-tableform-border-spacing: 0 var(--spectrum-spacing-300);
+  /* matching 20px, missing global token for 20px */


@pfulton We are missing 20px from the core-tokens. Is it ok to get 20px with a core-tokens addition?

Yeah, I think so. This will be something we'll need to keep an eye on, as I think I'd much rather have it coming as a definition from core tokens, but in the meantime (or if we have to), let's do this for now with an eye on possibly defining it ourselves in the custom-* files at some future point.

If we're missing 20px in spacing values, then perhaps the component was designed wrong (or core tokens needs to have a new spacing value). We shouldn't be calcing this.

components/fieldlabel/skin.css

components/tokens/custom-spectrum/custom-vars.css

pfulton

This is looking really great, @bernhard-adobe! Thanks so much for your work on this. I left a few comments here, and I think I answered the questions that you had left for me.

Two more things:

Let's be sure to add the --mod- prefixed custom properties in the places where we're using the other core tokens properties (look at ActionButton as your guide on this).
Let's be sure to add the --highcontrast prefixed custom properties in any places where we need high contrast mode to take effect.

pfulton · 2022-07-15T16:42:06Z

components/fieldlabel/index.css

+  --spectrum-fieldlabel-top-to-text: var(--spectrum-component-top-to-text-75);
+  --spectrum-fieldlabel-bottom-to-text: var(--spectrum-component-bottom-to-text-75);
+  --spectrum-fieldlabel-font-size: var(--spectrum-font-size-75);
+  --spectrum-fieldlabel-line-height: var(--spectrum-line-height-200);


Based on the Xd components file, I think the line-height remains the same for all of the t-shirt sizes (as spectrum-line-height-100). If that's correct (perhaps we should check with design), then I think we can move --spectrum-fieldlabel-line-height: var(--spectrum-line-height-100); to the .spectrum-FieldLabel selector.

I noticed in the inspection of the current docs site that the line-height changes.
size S and M = 15.6px
size L = 18.2px
Size XL = 20.8px

I think from a visual perspective you do want to increase the line-heights for t-shirt sizing to keep the relational "growth" in sizing.

@pfulton does this sound correct to you? Check in tomorrows Design Workshop maybe?

@pfulton I added the --mod modifiers. As there is no color-settings here I don't think we need to set --highcontrast

pfulton · 2022-07-15T16:47:00Z

components/fieldlabel/index.css

+  --spectrum-fieldlabel-top-to-text: var(--spectrum-component-top-to-text-100);
+  --spectrum-fieldlabel-bottom-to-text: var(--spectrum-component-bottom-to-text-100);
+  --spectrum-fieldlabel-font-size: var(--spectrum-font-size-100);
+  --spectrum-fieldlabel-line-height: var(--spectrum-CJK-line-height-100);


Similar to the comment above about line-height, but also, for this CJK value, we'll want to use a lang attribute selector to apply this.

The code will end up looking something like:

.spectrum-FieldLabel { --spectrum-fieldlabel-CJK-line-height: var(--spectrum-CJK-line-height-100); &:lang(ja), &:lang(zh), &:lang(ko) { line-height: var(--mod-fieldlabel-CJK-line-height, var(--spectrum-fieldlabel-CJK-line-height)); } }

Will add! Good feedback, thank you

added to line 99

/* international lang support */ .spectrum-FieldLabel { &:lang(ja), &:lang(zh), &:lang(ko) { line-height: var(--mod-fieldlabel-CJK-line-height, var(--spectrum-fieldlabel-CJK-line-height)); } }

pfulton · 2022-07-15T16:48:37Z

components/fieldlabel/index.css

+  --spectrum-fieldlabel-top-to-text: var(--spectrum-component-top-to-text-200);
+  --spectrum-fieldlabel-bottom-to-text: var(--spectrum-component-bottom-to-text-200);
+  --spectrum-fieldlabel-font-size: var(--spectrum-font-size-200);
+  --spectrum-fieldlabel-line-height: var(--spectrum-CJK-line-height-200);


I think this is the same as the previous comment (with the lang attribute), but also let's double-check with design as to whether this value changes with the t-shirt sizing, or if it stays consistent across the sizes.

same, lets double-check, maybe in a design workshop. I think we will discuss soon again the core-tokens for font-naming and weight and this might be a good additional question.

added support for international line-heights

components/fieldlabel/index.css

components/fieldlabel/skin.css

components/tokens/custom-spectrum/custom-vars.css

pfulton · 2022-07-15T17:03:59Z

components/fieldlabel/index.css

-  --spectrum-tableform-labelsabove-item-gap: var(--spectrum-global-dimension-size-100);
+  /* for /docs/form.html to set field-label inside form */
+  --spectrum-tableform-border-spacing: 0 var(--spectrum-spacing-300);
+  /* matching 20px, missing global token for 20px */


Yeah, I think so. This will be something we'll need to keep an eye on, as I think I'd much rather have it coming as a definition from core tokens, but in the meantime (or if we have to), let's do this for now with an eye on possibly defining it ourselves in the custom-* files at some future point.

lazd · 2022-07-20T22:26:56Z

components/fieldlabel/index.css

@@ -117,7 +134,7 @@ governing permissions and limitations under the License.
    flex-direction: column;

    & + .spectrum-Form-item {
-      margin-block-start: var(--spectrum-tableform-labelsabove-item-gap);
+      margin-block-start: var(--spectrum-spacing-100);


Any reason why this little fella isn't a component-level token above? It's not forbidden, per se, but I'd rather see it defined above for consistency.

@lazd I am actually not sure which item to use here.
Spacing 100 would be

"spacing-100": { "value": "8px" },

Maybe use this? I don't see any other component level tokens for field-label matching 8px.

"field-label-top-to-asterisk-small": { "sets": { "desktop": { "value": "8px" }, "mobile": { "value": "11px" } } },

I think you just need to define that as a component-level token in CSS:

.spectrum--medium { --spectrum-field-label-top-to-asterisk-small: 8px; } .spectrum--large { --spectrum-field-label-top-to-asterisk-small: 11px; }

Added, thanks @lazd

github-actions · 2022-07-25T20:16:02Z

🚀 Deployed on https://pr-1476--spectrum-css.netlify.app

pfulton

A few things here @bernhard-adobe - thanks so much for your hard work on this and your patience in waiting for my review.

pfulton · 2022-08-03T14:30:16Z

components/fieldlabel/package.json

@@ -17,22 +17,22 @@
  },
  "peerDependencies": {
    "@spectrum-css/icon": "^3.0.21",
-    "@spectrum-css/vars": "^8.0.0"
+    "@spectrum-css/tokens": "^1.0.1"


Let's bump this to the most recent release:

Suggested change

"@spectrum-css/tokens": "^1.0.1"

"@spectrum-css/tokens": "^1.0.4"

pfulton · 2022-08-03T14:30:30Z

components/fieldlabel/package.json

    "@spectrum-css/fieldgroup": "^3.1.4",
    "@spectrum-css/icon": "^3.0.23",
    "@spectrum-css/picker": "^1.2.12",
    "@spectrum-css/radio": "^3.0.24",
    "@spectrum-css/stepper": "^3.0.26",
    "@spectrum-css/textfield": "^3.2.4",
-    "@spectrum-css/vars": "^8.0.0",
+    "@spectrum-css/tokens": "^1.0.1",


Suggested change

"@spectrum-css/tokens": "^1.0.1",

"@spectrum-css/tokens": "^1.0.4",

pfulton · 2022-08-03T14:33:03Z

components/fieldlabel/index.css

+  /* for /docs/form.html to set field-label inside form */
+  --spectrum-tableform-border-spacing: 0 var(--spectrum-spacing-300);
+  /* matching 20px, missing global token for 20px */
+  --spectrum-tableform-margin-calc: calc(var(--mod--spacing-300, var(--spectrum-spacing-300)) + var(--mod--spacing-300, var(--spectrum-spacing-200)));


This is a place where we won't want to define the --mod- custom properties. Instead, we'd define them later when we actually use --spectrum-tableform-margin-calc. Does that make sense?

simplified!

components/fieldlabel/index.css

pfulton · 2022-08-03T15:05:18Z

One more thing, @bernhard-adobe: the color for the disabled variant looks pretty different than it did before (see below with current and previous). Would you mind double-checking on that color?

bernhard-adobe · 2022-08-11T22:17:00Z

One more thing, @bernhard-adobe: the color for the disabled variant looks pretty different than it did before (see below with current and previous). Would you mind double-checking on that color?

That is indeed missing as it was not documented in the "Components Batch 1.xd" file. I guess the color would be (--spectrum-disabled-content-color) like all the other disabled components.

bernhard-adobe · 2022-08-11T22:21:50Z

One more thing, @bernhard-adobe: the color for the disabled variant looks pretty different than it did before (see below with current and previous). Would you mind double-checking on that color?

That is indeed missing as it was not documented in the "Components Batch 1.xd" file. I guess the color would be (--spectrum-disabled-content-color) like all the other disabled components.

bernhard-adobe · 2022-08-11T22:34:51Z

rebuild Jenkins: https://spectrum-visual-regression.ci.corp.adobe.com/view/Spectrum%20CSS/job/css-vrt-test/37/

VRT results are: https://spectrum-visual-regression.ci.corp.adobe.com/view/Spectrum%20CSS/job/css-vrt-test/37/artifact/backstop_data/html_report/index.html

bernhard-adobe · 2022-08-11T22:35:39Z

@pfulton color changes (as expected) for disabled and line-height changes (as expected):

pfulton · 2022-08-23T13:37:59Z

Released beta:
5.0.0-beta.0

Co-authored-by: Monet Fort <lunarfusion@users.noreply.github.com> Co-authored-by: Patrick Fulton <pfulton@users.noreply.github.com>

* main: (53 commits) chore(release): release fix(checkbox): whcm focus states (#1527) chore(release): release feat(fieldlabel)!: migrate to core tokens (CSS-102) (#1476) chore(release): release feat(swatchgroup)!: migrate swatchgroup to core tokens (#1505) chore(release): release feat(swatch)!: migrate swatch to core tokens (#1501) chore(release): release fix(tabs): selection indicator scroll overflow border (#1513) chore(release): release feat(divider)!: migrate to core tokens chore(release): release refactor(checkbox): remove commented out code (#1524) chore(release): release feat(progresscircle)!: migrate to core tokens chore(release): release feat(checkbox)!: migrate checkbox component to core tokens (CSS-99) (#1465) chore(release): release fix(card): increase content area height when necessary ...

* main: (65 commits) chore(release): release chore!: use latest CSS tokens dependency refactor(swatch)!: remap core token aliases & rename aliases refactor(helptext)!: remap core token aliases & rename aliases refactor(radio)!: remap core token aliases & rename aliases refactor(checkbox)!: remap core token aliases & rename aliases refactor(switch)!: remap core token aliases & rename aliases refactor(actionbutton)!: remap core token aliases & rename aliases refactor(closebutton)!: remap core token aliases & rename aliases feat(tokens)!: use latest beta release chore(release): release refactor(inlinealert)!: migrate to core tokens (#1519) chore(release): release fix(checkbox): whcm focus states (#1527) chore(release): release feat(fieldlabel)!: migrate to core tokens (CSS-102) (#1476) chore(release): release feat(swatchgroup)!: migrate swatchgroup to core tokens (#1505) chore(release): release feat(swatch)!: migrate swatch to core tokens (#1501) ...

bernhard-adobe added the Component: Field Label label Jul 13, 2022

bernhard-adobe requested a review from pfulton July 13, 2022 23:29

bernhard-adobe changed the title ~~feat(fieldlabel): wip - begin core tokens migration~~ feat: field-label core-tokens implementation (CSS-102) Jul 13, 2022

bernhard-adobe force-pushed the bschmidt-pfulton/CSS-102-field-label-core-tokens branch from 6c21fc7 to 8dcedaf Compare July 13, 2022 23:33

bernhard-adobe commented Jul 13, 2022

View reviewed changes

pfulton changed the title ~~feat: field-label core-tokens implementation (CSS-102)~~ feat(fieldlabel)!: core-tokens implementation (CSS-102) Jul 14, 2022

pfulton requested changes Jul 15, 2022

View reviewed changes

lazd reviewed Jul 20, 2022

View reviewed changes

bernhard-adobe added a commit that referenced this pull request Jul 25, 2022

feat: addressing PR review feedback (#1476)

dad8297

github-actions bot temporarily deployed to pull request July 25, 2022 20:16 Inactive

bernhard-adobe requested review from lazd and pfulton July 27, 2022 17:12

pfulton requested changes Aug 3, 2022

View reviewed changes

github-actions bot temporarily deployed to pull request August 11, 2022 19:12 Inactive

github-actions bot temporarily deployed to pull request August 11, 2022 22:19 Inactive

bernhard-adobe requested a review from pfulton August 11, 2022 22:21

pfulton pushed a commit that referenced this pull request Aug 23, 2022

feat: addressing PR review feedback (#1476)

578dde4

pfulton force-pushed the bschmidt-pfulton/CSS-102-field-label-core-tokens branch from 4ab136d to 46053a4 Compare August 23, 2022 13:22

github-actions bot temporarily deployed to pull request August 23, 2022 13:26 Inactive

github-actions bot temporarily deployed to pull request August 23, 2022 13:31 Inactive

pfulton removed the request for review from lazd August 23, 2022 13:38

pfulton added the released-beta label Aug 23, 2022

pfulton added the hacktoberfest-accepted label Oct 5, 2022

pfulton and others added 21 commits October 11, 2022 15:31

feat(fieldlabel): wip - begin core tokens migration

44a4d50

fix: temp disable missing font-sizes

5304832

fix: temp disabling missing core tokens

f6decfb

Co-authored-by: Monet Fort <lunarfusion@users.noreply.github.com> Co-authored-by: Patrick Fulton <pfulton@users.noreply.github.com>

fix: fixing t-shirt sizing for fieldlabel

fa185e9

fix: more spacing updates

e0ecf8c

fix: removing missing tokens

7c0d33a

feat: more soft-coding of core vars

60abe16

feat: all core-tokens for fieldlabel

3ba0c88

chore: removing unused code

088985a

feat: addressing PR review feedback (#1476)

91d8b2a

feat: adding mod modifiers for fieldlabel

fe02bc9

feat: swapping out top-top-askterisk-small to temp hard-corded tokens

c9e2f0c

feat: adding back disabled state and upping package

45cdeb7

chore(fieldlabel): use latest versions of dependencies

6db64a0

chore(fieldlabel): manually bump version number for beta release

2161d70

chore(fieldlabel): add empty custom selectors & bump version

bb75491

fix(fieldlabel): resolve syntax error & re-release

de05cd7

feat: correct padding-top for side-label

ea9e71e

feat: using core-tokens again for padding-top

6e012ba

chore(fieldlabel): manual version bump for beta release

8d36860

feat: upping core-tokens to v3.0.0

0e3f48e

pfulton force-pushed the bschmidt-pfulton/CSS-102-field-label-core-tokens branch from f9c7ba8 to 0e3f48e Compare October 11, 2022 19:38

github-actions bot temporarily deployed to pull request October 11, 2022 19:42 Inactive

chore(tokens): revert files to main state to skip lerna versioning

7e534df

github-actions bot temporarily deployed to pull request October 11, 2022 19:51 Inactive

pfulton merged commit 45802a6 into main Oct 11, 2022

pfulton deleted the bschmidt-pfulton/CSS-102-field-label-core-tokens branch October 11, 2022 19:55

	"@spectrum-css/tokens": "^1.0.1"
	"@spectrum-css/tokens": "^1.0.4"

	"@spectrum-css/tokens": "^1.0.1",
	"@spectrum-css/tokens": "^1.0.4",

feat(fieldlabel)!: core-tokens implementation (CSS-102) #1476

feat(fieldlabel)!: core-tokens implementation (CSS-102) #1476

Uh oh!

Conversation

bernhard-adobe commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Screenshots

To-do list

Uh oh!

netlify bot commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for spectrum-css ready!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pfulton commented Aug 3, 2022

Uh oh!

bernhard-adobe commented Aug 11, 2022

bernhard-adobe commented Jul 13, 2022 •

edited

Loading

netlify bot commented Jul 13, 2022 •

edited

Loading

github-actions bot commented Jul 25, 2022 •

edited

Loading