chore: make the spectrum-two theme fully functional #4731

graynorton · 2024-09-06T21:29:04Z

Description

The spectrum-two theme didn't yet have all of the necessary plumbing in tools/styles and tools/theme.

This PR adds the missing files and updates various build scripts to create a fully functioning spectrum-two theme.

Note that the spectrum-two theme (like the spectrum and express themes) includes the core-global CSS variables to ensure that application views relying on these variables will not break when migrating to the spectrum-two theme. Per the plan of record, these variables remain deprecated and will be removed in a future major version of SWC.

Related issue(s)

SWC-348, SWC-466 (internal JIRA)

Motivation and context

Allow the spectrum-two theme to be used and tested.

How has this been tested?

Tested in the Storybook theme switcher, in both SWC and the downstream UEC project.

…bute

… into jnjosh/theming-systems

…bute

…ctrum-web-components into jnjosh/theming-systems

… theme

… system

rubencarvalho

Great work! I left a few nit comments and a larger question around lightest and darkest artifacts 👍

rubencarvalho · 2024-09-11T22:38:15Z

scripts/spectrum-vars.js

@@ -138,6 +138,7 @@ const processTypography = async (
 // TODO: use resolve package to find node_modules
 // TODO: we need to revisit whether we need these
 const spectrumPaths = [
+    // Spectrum 1
    path.resolve(
        path.join(
            __dirname,


nit: could we clean up some repetition and avoid using indexes below? For example, instead of relying on the i values, we could structure the spectrumPaths as objects with descriptive fields like system.

const CSSbasePath = path.resolve(path.join(__dirname, '..', 'node_modules', '@spectrum-css')); const spectrumPaths = [ // Spectrum 1 { path: path.join(CSSbasePath, 'vars', 'dist'), system: 'spectrum-one' }, // Spectrum 2 (same path as S1) { path: path.join(CSSbasePath, 'vars', 'dist'), system: 'spectrum-two' }, // Express { path: path.join(CSSbasePath, 'expressvars', 'dist'), system: 'express' } ];

Yes, this would definitely be nicer!

My high-level approach with this PR was to apply as light a touch as possible, doing just what was necessary to make the new theme work, but it seems worth doing this to leave things in a cleaner state. I'll go ahead and make the change.

Done! I started by applying your suggestion directly, then ended up simplifying a bit more in the process of changing to use import.meta.resolve to find the @spectrum-css packages.

rubencarvalho · 2024-09-11T22:48:28Z

scripts/spectrum-vars.js

@@ -170,10 +183,19 @@ const foundVars = await findUsedVars();

 spectrumPaths.forEach((spectrumPath, i) => {


this could then look like

spectrumPaths.forEach(({ path, system }) => { ... }

rubencarvalho · 2024-09-11T22:50:03Z

tools/styles/spectrum-two/core-global.css

+
+:host,
+:root {
+    -webkit-tap-highlight-color: rgb(0 0 0 / 0%);


I see this is also on the other systems, but why do we need this? 🤔

Before we decide to remove this can we confirm if the tap effect coming up in iOS?

That's a good question; again, my "make it work" approach to this PR led me to not pull on this thread at the moment, but we can certainly try to figure it out.

@Rajdeepc, @jnjosh, @nikkimk, does any of you know?

Not that I know of right now but I will try to dig into the glossary. But this shouldn't block the merge :)

This comment is mostly for me to get more context. It is non-blocking.

rubencarvalho · 2024-09-11T22:52:13Z

tools/theme/package.json

+            "development": "./spectrum-two/theme-light.dev.js",
+            "default": "./spectrum-two/theme-light.js"
+        },
+        "./spectrum-two/theme-lightest-core-tokens.js": {


AFAIK there is no lightest theme on S2.

I was following the lead of the Express system, which also doesn't have lightest or darkest themes but nonetheless includes files for loading lightest and darkest. These files actually actually just load the light and dark values under the hood.

My assumption was that this was done to avoid failing ungracefully in the case where someone populates the system and color properties of <sp-theme> with a non-existent pairing.

My instinct is that a more principled approach would be to rework <sp-theme> so that you simply get dev mode warnings when trying invalid combinations, rather than using this "facade" approach within each theme...but a) I'm not sure whether any customers of the Express theme might be relying on the status quo; and b) again, my general approach to this PR was to get the spectrum-two theme working with minimal changes to the code.

I am open to going a different way here, but my instinct remains to take a light touch for now, on the assumption that we'll likely end up making significant changes to the whole theming mechanism in the 1.x => 2.0 transition, and that would be a better time to revisit the approach here.

This exports are a miss from our side while deprecating lightest and darkest theme. We can tackle it in the main PR. Thanks

Understood, and that does make sense. I agree with the "let's keep changes to a minimum" approach for now. It's definitely something we can address later on.

If nothing else, this provides yet another good data point for us to look at, whenever we take an holistic approach to theming, "systems", sp-theme,...

tools/theme/src/Theme.ts

rubencarvalho · 2024-09-11T22:53:43Z

tools/theme/src/spectrum-two/themes-core-tokens.ts

@@ -11,6 +11,8 @@ governing permissions and limitations under the License.
 */

 import '../../spectrum-two/theme-light-core-tokens.js';
+import '../../spectrum-two/theme-lightest-core-tokens.js';


double check: lightest and darkest should not be needed?

tools/theme/src/spectrum-two/themes.ts

rubencarvalho · 2024-09-11T22:58:37Z

packages/action-group/package.json

@@ -58,7 +58,7 @@
        "lit-html"
    ],
    "dependencies": {
-        "@lit-labs/observers": "^2.0.0",
+        "@lit-labs/observers": "^2.0.2",


might as well go for the latest 2.0.3 version? unless there is a transitive dependency conflict :)

I only made this change to ensure that all packages within SWC were pulling in the same version of observers; the others were on 2.0.2 already.

The reason why I went down this path at all in this otherwise-unrelated PR is that the discrepancy was causing yarn link to fail when linking UEC and SWC for testing purposes; making this change allowed yarn link to work! 🎉

We could consider bumping everything to the latest version, but that seemed like a truly unrelated change best done in a separate PR.

Yes we can create a follow up PR to version bumps all the dependencies. We usually have one planned from dependebot after we merge colors to main. This can stay at the current version only if you wish to!

Thanks for the context. Let us keep this as it is - I was mostly wondering if you had faced other upgrade-related conflicts or difficulties.

rubencarvalho · 2024-09-11T23:03:39Z

scripts/spectrum-vars.js

@@ -138,6 +138,7 @@ const processTypography = async (
 // TODO: use resolve package to find node_modules


nit: on a side note, we could tackle this TODO and consider using resolve to dynamically locate the node_modules directory instead of hardcoding the paths below. Not sure if it is very needed, but I think this could help with symlinked packages.

Yeah, I thought about that as well, but opted not to in keeping with my "lightest touch" approach to this PR. Let me know if you feel strongly about making this change now and we can discuss.

I actually went ahead and took care of this when incorporating the other change you suggested. 🙌

I see, thanks! 😄 And who knows - maybe it actually will make our future selves happy someday!

graynorton · 2024-09-12T21:34:27Z

Great work! I left a few nit comments and a larger question around lightest and darkest artifacts 👍

Thanks for the thorough review and suggestions, @rubencarvalho!

I took care of the nits and commented with my rationale for including the lightest and darkest "facades".

…heme

rubencarvalho

Thank you for the quick turn-around! 👍

TarunAdobe

I'd love to discuss the benefits of supporting non-core styles and lightest/darkest in S2. Both of these things we've marked deprecated in S1/Express and I don't want us to unconsciously promote consumers to use these types in S2 at least. S2 is not yet fully supported either way and I kinda feel okay if someone breaks their system because they were using lightest with S2. We already give dev mode warnings for this so I don't see why we're adding support for this in S2. I might be missing something bt this was basically my thought process while working on the testing-css-bridge-for-s2 branch few months back.

Rajdeepc

Good PR!

Rajdeepc · 2024-09-09T08:36:59Z

scripts/spectrum-vars.js

    themes.forEach((theme) => {
-        if (isExpress && ['lightest', 'darkest'].includes(theme)) return;
+        if (!isSpectrum1 && ['lightest', 'darkest'].includes(theme)) {


lightest and darkest will be deprecated in 1.0! Can we include light and dark only?

Rajdeepc · 2024-09-09T08:39:16Z

tools/theme/spectrum-two/theme-darkest.ts

+import { Theme } from '../src/Theme.js';
+import '../src/spectrum-two/core.js';
+
+Theme.registerThemeFragment('darkest-spectrum-two', 'color', darkStyles);


darkest will be deprecated in 1.0! Should this be dark?

Rajdeepc · 2024-09-09T08:39:34Z

tools/theme/spectrum-two/theme-darkest-core-tokens.ts

+import { Theme } from '../src/Theme.js';
+import '../src/spectrum-two/core-tokens.js';
+
+Theme.registerThemeFragment('darkest-spectrum-two', 'color', darkStyles);


darkest will be deprecated in 1.0! Should this be dark?

Rajdeepc · 2024-09-12T09:20:18Z

projects/story-decorator/src/StoryDecorator.ts

@@ -25,7 +25,7 @@ import {
 import { DARK_MODE } from '@spectrum-web-components/reactive-controllers/src/MatchMedia.js';
 import '@spectrum-web-components/theme/sp-theme.js';
 import '@spectrum-web-components/theme/src/themes.js';
-import '@spectrum-web-components/theme/src/spectrum-two/themes-core-tokens.js';
+import '@spectrum-web-components/theme/src/spectrum-two/themes.js';


This needs to be updated in core-tokens.md also

Rajdeepc · 2024-09-12T09:23:39Z

tools/styles/spectrum-two/core-global.css

+
+:host,
+:root {
+    -webkit-tap-highlight-color: rgb(0 0 0 / 0%);


Before we decide to remove this can we confirm if the tap effect coming up in iOS?

Rajdeepc · 2024-09-13T14:16:48Z

tools/styles/spectrum-two/core-global.css

+
+:host,
+:root {
+    -webkit-tap-highlight-color: rgb(0 0 0 / 0%);


Not that I know of right now but I will try to dig into the glossary. But this shouldn't block the merge :)

Rajdeepc · 2024-09-13T14:22:26Z

tools/theme/package.json

+            "development": "./spectrum-two/theme-light.dev.js",
+            "default": "./spectrum-two/theme-light.js"
+        },
+        "./spectrum-two/theme-lightest-core-tokens.js": {


This exports are a miss from our side while deprecating lightest and darkest theme. We can tackle it in the main PR. Thanks

Rajdeepc · 2024-09-13T14:23:34Z

packages/action-group/package.json

@@ -58,7 +58,7 @@
        "lit-html"
    ],
    "dependencies": {
-        "@lit-labs/observers": "^2.0.0",
+        "@lit-labs/observers": "^2.0.2",


Yes we can create a follow up PR to version bumps all the dependencies. We usually have one planned from dependebot after we merge colors to main. This can stay at the current version only if you wish to!

graynorton · 2024-09-16T18:07:41Z

I'd love to discuss the benefits of supporting non-core styles and lightest/darkest in S2.

@TarunAdobe

As for including the non-core (global) tokens in the spectrum-two system, this is something that I've discussed extensively with @jnjosh, @rubencarvalho and @Rajdeepc, and we've concluded that these tokens should remain in "deprecated" status for SWC 1.x (across all three systems) and be removed in SWC 2.0.

The rationale is that we want to get customers to spectrum-two as quickly and painlessly as possible, then support them in completing larger migration tasks during the SWC 1.x => 2.0 transition (which we expect to last for a year or more). If we were to ship the spectrum-two system in SWC 1.0 without including the legacy tokens, then teams would face potentially significant migration work before they could even try out and adopt spectrum-two.

As for including lightest and darkest in spectrum-two, please see the discussion with @rubencarvalho and @Rajdeepc elsewhere in this PR review. The tl;dr is that I followed the example of the express theme, which I believe is in exactly the same situation as spectrum-two (the system itself does not include lightest and darkest tokens, but the theme is set up to "fall back" to the light and dark tokens if a user sets color to lightest or darkest). We could reasonably make a different choice here, but I don't think that needs to be handled as part of this PR.

Does this make sense?

Rajdeepc · 2024-09-18T10:08:00Z

I'd love to discuss the benefits of supporting non-core styles and lightest/darkest in S2.

@TarunAdobe

As for including the non-core (global) tokens in the spectrum-two system, this is something that I've discussed extensively with @jnjosh, @rubencarvalho and @Rajdeepc, and we've concluded that these tokens should remain in "deprecated" status for SWC 1.x (across all three systems) and be removed in SWC 2.0.

The rationale is that we want to get customers to spectrum-two as quickly and painlessly as possible, then support them in completing larger migration tasks during the SWC 1.x => 2.0 transition (which we expect to last for a year or more). If we were to ship the spectrum-two system in SWC 1.0 without including the legacy tokens, then teams would face potentially significant migration work before they could even try out and adopt spectrum-two.

As for including lightest and darkest in spectrum-two, please see the discussion with @rubencarvalho and @Rajdeepc elsewhere in this PR review. The tl;dr is that I followed the example of the express theme, which I believe is in exactly the same situation as spectrum-two (the system itself does not include lightest and darkest tokens, but the theme is set up to "fall back" to the light and dark tokens if a user sets color to lightest or darkest). We could reasonably make a different choice here, but I don't think that needs to be handled as part of this PR.

Does this make sense?

Thanks for the explanation! I feel lightest and darkest are not needed to be shipped as a part of spectrum-two reference since these needs to be deleted in 1.0. We would also want to surface this information to the consumers like express to not to use lightest and darkest global tokens and we have already deleted these two color modes already! I am okay to get these shipped as part of S1 but S2 will not have any global definitions of lightest and darkest.

graynorton · 2024-09-18T15:30:01Z

I feel lightest and darkest are not needed to be shipped as a part of spectrum-two reference since these needs to be deleted in 1.0.

Sure, that makes sense to me. My suggestion would be to merge this PR, and then follow up with a new PR to remove lightest and darkest from both express and spectrum-two.

Rajdeepc · 2024-09-18T16:32:53Z

I feel lightest and darkest are not needed to be shipped as a part of spectrum-two reference since these needs to be deleted in 1.0.

Sure, that makes sense to me. My suggestion would be to merge this PR, and then follow up with a new PR to remove lightest and darkest from both express and spectrum-two.

Sounds great! I think we are all in the same page now! Thanks everyone

graynorton · 2024-10-24T20:28:08Z

Closing this PR in favor of the cleanly reconstructed #4859

jnjosh and others added 30 commits March 15, 2024 11:51

refactor(theme): adds new system attribute and deprecates theme attri…

2983a36

…bute

chore(theme): made changes on theme attribute

956106f

chore: reverted THEME Key to support migrations

407efbf

Merge branch 'main' of https://github.com/adobe/spectrum-web-components…

f7c2486

… into jnjosh/theming-systems

chore: theme registrated hard break

664d2c2

refactor(theme): adds new system attribute and deprecates theme attri…

9714b15

…bute

chore(theme): made changes on theme attribute

4830eed

chore: reverted THEME Key to support migrations

e69bb48

chore: theme registrated hard break

dc00e12

Merge branch 'jnjosh/theming-systems' of https://github.com/adobe/spe…

a20edc2

…ctrum-web-components into jnjosh/theming-systems

chore(theme): updated theme property to system

e159afc

chore(theme): updated theme to system in sp-theme in layout

576b603

chore(theme): updated config for VRT

4ffc9fb

chore(theme): updated config for VRT from theme to system

bfa82d9

chore: update theme property to system

20595ee

fix: added spectrum system variant

6fe7a68

chore: updated theme for deprecation against system in theme files

2490dd5

chore: updated config to support system

8830483

chore: updated config yml

be29dcc

chore: updated config yml

1a4624e

chore: updated config yml

4ea39f0

chore: undo system changes in stories

36bf032

chore: updated runner utils

771a7be

chore: review to run with theme

c3625ae

chore(theme): updated command-line arguments to ue system instea do f…

e9da66d

… theme

chore(theme): updated command-line arguments to ue system instea do f…

6c413b0

… theme

chore(theme): updated and cleaned up reduntant theme definitions with…

d26d278

… system

chore: added theme fallback to the storydecotrator render

11a8e6d

chore: added system flag in the create review site job in CI

86e1fec

chore: added system flag in the create review site job in CI

32da346

graynorton and others added 4 commits September 11, 2024 08:30

chore: update StoryDecorator to load full S2 theme

163aba2

chore: fixed yarn.lock

5d3a361

ci: updated golden image cache

35c9e02

chore: clean up after rebase

b80ffc3

graynorton force-pushed the complete-spectrum-two-theme branch from e082137 to b80ffc3 Compare September 11, 2024 18:40

graynorton changed the base branch from main to testing-css-bridge-for-s2 September 11, 2024 18:41

graynorton marked this pull request as ready for review September 11, 2024 20:38

graynorton requested a review from a team as a code owner September 11, 2024 20:38

rubencarvalho reviewed Sep 11, 2024

View reviewed changes

TarunAdobe and others added 4 commits September 12, 2024 13:38

chore: fixed action-button config

84c8f62

chore: action button edge to visual custom css

a7b9620

fix(picker): picker CSS does not need to set '--spectrum-icon-size'

cd8f739

chore: clean up logic and package resolution

0a64cc3

graynorton requested a review from rubencarvalho September 12, 2024 21:35

TarunAdobe self-requested a review September 13, 2024 03:58

TarunAdobe changed the title ~~Make the spectrum-two theme fully functional~~ chore: make the spectrum-two theme fully functional Sep 13, 2024

TarunAdobe and others added 2 commits September 13, 2024 13:20

chore: updated spectrum-css versions to latest release

a5c9758

Merge branch 'testing-css-bridge-for-s2' into complete-spectrum-two-t…

d130942

…heme

rubencarvalho approved these changes Sep 14, 2024

View reviewed changes

TarunAdobe requested changes Sep 14, 2024

View reviewed changes

Rajdeepc approved these changes Sep 16, 2024

View reviewed changes

Base automatically changed from testing-css-bridge-for-s2 to feature/SWC-1.0-RC October 16, 2024 15:52

graynorton mentioned this pull request Oct 24, 2024

chore: make the spectrum-two theme fully functional #4859

Merged

graynorton closed this Oct 24, 2024

		@@ -170,10 +183,19 @@ const foundVars = await findUsedVars();

		spectrumPaths.forEach((spectrumPath, i) => {

		@@ -138,6 +138,7 @@ const processTypography = async (
		// TODO: use resolve package to find node_modules

chore: make the spectrum-two theme fully functional #4731

chore: make the spectrum-two theme fully functional #4731

Conversation

graynorton commented Sep 6, 2024 • edited Loading

Description

Related issue(s)

Motivation and context

How has this been tested?

rubencarvalho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graynorton commented Sep 12, 2024 • edited Loading

rubencarvalho left a comment

Choose a reason for hiding this comment

TarunAdobe left a comment

Choose a reason for hiding this comment

Rajdeepc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graynorton commented Sep 16, 2024

Rajdeepc commented Sep 18, 2024

graynorton commented Sep 18, 2024

Rajdeepc commented Sep 18, 2024

graynorton commented Oct 24, 2024

graynorton commented Sep 6, 2024 •

edited

Loading

graynorton commented Sep 12, 2024 •

edited

Loading