refactor(inlinealert)!: migrate core tokens #1519

yosevu · 2022-09-13T19:25:41Z

Description

Removed Help variant since it wasn't in the design
Renamed success variant to positive to match design
Renamed error variant to notice to match design
Reordered variants
Added WHCM styles

Does not include

Typography components
Updated min-width token

How and where has this been tested?

Chrome and Safari
WHCM

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.

github-actions · 2022-09-13T19:30:25Z

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

components/inlinealert/index.css

lunarfusion

I think the skin file needs to be deleted and the branch needs to be rebased to main.

Looks good!

components/inlinealert/index.css

lunarfusion · 2022-09-26T17:57:28Z

components/inlinealert/index.css

+  align-items: center;
+  justify-content: space-between;
+  /* Minimum space between header and icon */
+  gap: var(--mod-inlinealert-spacing-header-to-icon, var(--spectrum-inlinealert-spacing-header-to-icon));


Excellent choice to use gap instead of inline margin.

lunarfusion · 2022-09-26T19:02:33Z

components/inlinealert/index.css

 }

 .spectrum-InLineAlert-content {
  display: block;
-  margin: var(--spectrum-inlinealert-neutral-content-margin-top) 0 0 0;
+  margin: var(--mod-inlinealert-spacing-header-content-button, var(--spectrum-inlinealert-spacing-header-content-button)) 0 0 0;


Should we change this to margin-block-start and get rid of the '0 0 0'?

I agree with @lunarfusion on this. Logical properties (currently) don't have the ability to do short-hand value definitions, so you'll (probably) want to change this to margin-block-start, margin-block-end, margin-inline-start, and margin-inline-end.

components/inlinealert/index.css

components/inlinealert/package.json

pfulton

This is looking good, @yosevu - I left some comments here that we should address.

components/inlinealert/package.json

pfulton · 2022-10-03T14:57:43Z

components/inlinealert/index.css

-  border-radius: var(--spectrum-inlinealert-neutral-corner-radius);
+  border-radius: var(--mod-inlinealert-border-radius, var(--spectrum-inlinealert-border-radius));
+
+  background-color: var(--mod-inlinealert-background-color, var(--spectrum-inlinealert-background-color));


Can you add the var(--highcontrast-inlinealert-background-color) custom property here too, please?

components/inlinealert/index.css

pfulton · 2022-10-03T15:02:26Z

components/inlinealert/index.css

 }

 .spectrum-InLineAlert-content {
  display: block;
-  margin: var(--spectrum-inlinealert-neutral-content-margin-top) 0 0 0;
+  margin: var(--mod-inlinealert-spacing-header-content-button, var(--spectrum-inlinealert-spacing-header-content-button)) 0 0 0;


I agree with @lunarfusion on this. Logical properties (currently) don't have the ability to do short-hand value definitions, so you'll (probably) want to change this to margin-block-start, margin-block-end, margin-inline-start, and margin-inline-end.

pfulton · 2022-10-03T15:02:48Z

components/inlinealert/index.css

  word-wrap: break-word;
+
+  color: var(--mod-inlinealert-header-and-content-color, var(--spectrum-inlinealert-header-and-content-color));


Can you add the --highcontrast custom property here?

components/inlinealert/index.css

yosevu · 2022-10-04T07:07:08Z

@pfulton I had paused work on this while waiting for typography so I didn't finish adding all the token fallbacks. I have added them all now though and I'm thinking it might make sense to do the Typography component integrations a separate issue if possible.

pfulton · 2022-10-04T14:41:27Z

@pfulton I had paused work on this while waiting for typography so I didn't finish adding all the token fallbacks. I have added them all now though and I'm thinking it might make sense to do the Typography component integrations a separate issue if possible.

Yeah, I think that makes sense to me.

pfulton · 2022-10-04T14:44:02Z

components/inlinealert/index.css

+  .spectrum-InLineAlert {
+    --highcontrast-inlinealert-background-color: Background;
+    --highcontrast-inlinealert-header-and-content-color: CanvasText;
+    --highcontrast-inlinealert-border-and-icon-color: ButtonBorder;
+  }


@yosevu I missed this in my previous review, so I made the change myself. I think we should try to stick with scoping the highcontrast custom properties to the component's selector. Previously, (without the selector) these custom properties were just being defined without any selector scope.

pfulton · 2022-10-04T14:48:47Z

All right, this is rebased against the latest stuff in main and the beta has been published @yosevu:

@spectrum-css/inlinealert@5.0.0-beta.0

pfulton

Would you mind taking a look at this in Firefox? The borders are looking a little funky:

I think you might need to also define border-inline-width in addition to your existing border-block-width (at least, that's what I tried in my browser inspector and it seemed to work ok?)

yosevu · 2022-10-06T20:28:12Z

Would you mind taking a look at this in Firefox? The borders are looking a little funky:

I think you might need to also define border-inline-width in addition to your existing border-block-width (at least, that's what I tried in my browser inspector and it seemed to work ok?)

Updated. I wonder why we didn't see the issue in Chrome.

BEAKING CHANGE

Contain icon within header and use flexbox.

Font styles will be handled in Typography Heading

pfulton

This looks good now, @yosevu! I also rebased this against main and bumped the beta version number. It's been released at: @spectrum-css/inlinealert@5.0.0-beta.1

* 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) ...

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from b6b7c94 to ddf3a82 Compare September 13, 2022 19:30

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from ddf3a82 to 1b7bd93 Compare September 13, 2022 19:33

github-actions bot temporarily deployed to pull request September 13, 2022 19:37 Inactive

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from 1b7bd93 to 32b6658 Compare September 13, 2022 19:37

github-actions bot temporarily deployed to pull request September 13, 2022 19:42 Inactive

yosevu commented Sep 13, 2022

View reviewed changes

components/inlinealert/index.css Outdated Show resolved Hide resolved

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from 67b3c9e to faf7b76 Compare September 13, 2022 20:05

github-actions bot temporarily deployed to pull request September 13, 2022 20:09 Inactive

yosevu requested review from lunarfusion, pfulton and bernhard-adobe September 13, 2022 20:17

lunarfusion reviewed Sep 26, 2022

View reviewed changes

pfulton reviewed Oct 3, 2022

View reviewed changes

components/inlinealert/package.json Outdated Show resolved Hide resolved

pfulton requested changes Oct 3, 2022

View reviewed changes

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from 591db8b to e977c6a Compare October 4, 2022 07:20

yosevu requested a review from pfulton October 4, 2022 07:22

github-actions bot temporarily deployed to pull request October 4, 2022 07:24 Inactive

pfulton force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from e977c6a to 75c15a0 Compare October 4, 2022 14:40

pfulton approved these changes Oct 4, 2022

View reviewed changes

pfulton reviewed Oct 4, 2022

View reviewed changes

github-actions bot temporarily deployed to pull request October 4, 2022 14:44 Inactive

pfulton added released-beta labels Oct 4, 2022

yosevu requested a review from pfulton October 5, 2022 11:18

yosevu force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from 75c15a0 to bf5e131 Compare October 5, 2022 11:26

github-actions bot temporarily deployed to pull request October 5, 2022 11:30 Inactive

github-actions bot temporarily deployed to pull request October 5, 2022 11:36 Inactive

pfulton added the hacktoberfest-accepted label Oct 5, 2022

pfulton requested changes Oct 5, 2022

View reviewed changes

yosevu requested a review from pfulton October 6, 2022 20:27

github-actions bot temporarily deployed to pull request October 6, 2022 20:30 Inactive

Yosevu Kilonzo and others added 17 commits October 12, 2022 10:26

refactor(inlinealert): migrate core tokens

07a3ece

BEAKING CHANGE

refactor: update variants

df8dfeb

feat: add token fallbacks

0090c64

chore: review removed styles

2305e03

fix: update header to icon minimum spacing

4131129

Contain icon within header and use flexbox.

feat: add static properties for missing tokens

8123440

chore: update example text

a6bd522

fix: add font-weight token

c4e906e

chore: update copyright

a537d07

chore: update dependencies

9786ea4

feat: add highcontrast tokens

3a79d7b

feat: add missing mod tokens

71febe1

fix: use logical margin properties

03333f8

chore: remove unused styles

c5066a7

Font styles will be handled in Typography Heading

fix: define WHCM tokens within selector

c743331

fix: add border-inline-width

8db1584

chore(inlinealert): manual version bump for updated beta release

3893238

pfulton force-pushed the yosevu/css-152-migrate-inline-alert-to-core-tokens branch from 74afa27 to 3893238 Compare October 12, 2022 14:30

github-actions bot temporarily deployed to pull request October 12, 2022 14:34 Inactive

pfulton approved these changes Oct 12, 2022

View reviewed changes

pfulton merged commit a4d72ad into main Oct 12, 2022

pfulton deleted the yosevu/css-152-migrate-inline-alert-to-core-tokens branch October 12, 2022 21:04

		word-wrap: break-word;

		color: var(--mod-inlinealert-header-and-content-color, var(--spectrum-inlinealert-header-and-content-color));

refactor(inlinealert)!: migrate core tokens #1519

refactor(inlinealert)!: migrate core tokens #1519

Uh oh!

Conversation

yosevu commented Sep 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!

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

Uh oh!

Uh oh!

Uh oh!

lunarfusion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lunarfusion Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

lunarfusion Sep 26, 2022

Choose a reason for hiding this comment

Uh oh!

pfulton Oct 3, 2022

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!

Uh oh!

pfulton Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pfulton Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

pfulton Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yosevu commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pfulton commented Oct 4, 2022

Uh oh!

pfulton Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

pfulton commented Oct 4, 2022

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

yosevu commented Oct 6, 2022

Uh oh!

pfulton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yosevu commented Sep 13, 2022 •

edited

Loading

github-actions bot commented Sep 13, 2022 •

edited

Loading

yosevu commented Oct 4, 2022 •

edited

Loading