refactor(card): rename mod and refactor some usage of mods and vars #2444

jawinn · 2024-01-17T22:07:39Z

Description

fix(card): rename misnamed instance of background color mod

The custom mod property for the background color was named incorrectly in one of the declarations.
This mod renaming was made in the reverted PR #2417 , which had some issues with the Horizontal variant. This does an additional bit of refactoring cleanup to address this:

refactor(card): cleanup mods and vars around use of preview bg

The custom properties and mods were not differentiating between the card background and the preview div background used in the quiet, gallery, and horizontal variations. This did not work well with the horizontal variant which has both a card background and a preview background, when some of the mis-named mods were fixed.

A few unset custom properties with spectrum prefixed naming also appeared to be intended as mods, and now have mod equivalents. These did not exist in the repo or in the tokens package, and have been removed/replaced:
--spectrum-card-background-color-quiet
--spectrum-card-background-color-hover-quiet
--spectrum-card-content-margin-top-quiet
--spectrum-card-minimum-width-quiet

This PR creates new custom properties for the preview background colors, that were previously being set through changing a mod value. It also renames --spectrum-card-preview-border-width to --spectrum-card-preview-border-width-selected to match with its usage.

A fallback was kept to maintain the use of --mod-card-background-color for the quiet variants. The horizontal variant now uses --mod-card-preview-background-color, as it has both a card background color and a "preview" element background color.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

No visual changes to Card variations in docs site, including focus and hover
No visual changes to Card in Storybook, including focus, hover, selected
Set --mod-card-background-color to a color and it should change the background color (does not in prod). Setting the deprecated fallback --mod-spectrum-card-background-color should also change it.
Setting --mod-card-preview-background-color should change the preview area bg color for quiet, and horizontal variants
Setting --mod-card-preview-background-color-hover should change the preview area bg color for quiet variants

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
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.
✨ This pull request is ready to merge. ✨

github-actions · 2024-01-17T22:16:06Z

File metrics

Summary

Total size: 4.51 MB^*
Total change (Δ): ⬆ 0.67 KB (0.01%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
card	22.53 KB	⬆ 0.19 KB

Details

card

File	Head	Base	Δ
index-base.css	22.53 KB	22.35 KB	⬆ 0.19 KB (0.83%)
index-vars.css	22.53 KB	22.35 KB	⬆ 0.19 KB (0.83%)
index.css	22.53 KB	22.35 KB	⬆ 0.19 KB (0.83%)
metadata.json	9.50 KB	9.40 KB	⬆ 0.10 KB (1.02%)

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

jawinn · 2024-01-17T22:19:07Z

components/card/index.css

-				var(--spectrum-card-background-color)
+				--mod-card-preview-background-color, 
+				var(
+					--mod-card-background-color, 


Should I mark the backwards compatible fallback --mod-card-background-color as being deprecated here? It was setting the background on the preview elements, while the main card background is set to transparent directly.

I kinda like this hierarchy, reminds me of a font stack. Most specific is a preview-namespaced mod, then the general card mod, then the variable. So I don't mind it as is, but there's also not really a precedent for it in our components as far as I know. Not really an answer, just thoughts 😁

What I was thinking is that ideally it seems like the gallery and quiet variations should be setting --mod-card-background-color: transparent in the CSS instead of background-color: transparent;, and then the --mod-card-preview-background-color would be used to set the background color. That seems to fit with our existing conventions and keeps the general bg color mod available for gallery. That would break any existing usage of --mod-card-background-color though.

github-actions · 2024-01-29T20:02:28Z

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

mdt2

Looks great. Just a couple questions/thoughts.

mdt2 · 2024-01-29T20:08:50Z

components/card/index.css

@@ -824,21 +842,20 @@ governing permissions and limitations under the License.
 			var(--spectrum-card-horizontal-preview-padding)
 		);
 		background-color: var(
-			--mod-card-background-color,
-			var(--spectrum-card-background-color)
+			--mod-card-preview-background-color,


Is there a reason we wouldn't include both --mod-card-preview-background-color and --mod-card-background-color here?

Yeah, this is tied to the general issue around the mod renaming. Here's a visual to better demonstrate:

--mod-card-preview-background-color: red; --mod-card-background-color: lightblue;

These are two separate background colors, and the left side defaults to a different grey that I wouldn't expect to also change if changing the main bg color (that changes the right side).

mdt2 · 2024-01-29T20:18:12Z

components/card/index.css

-				var(--spectrum-card-background-color)
+				--mod-card-preview-background-color, 
+				var(
+					--mod-card-background-color, 


I kinda like this hierarchy, reminds me of a font stack. Most specific is a preview-namespaced mod, then the general card mod, then the variable. So I don't mind it as is, but there's also not really a precedent for it in our components as far as I know. Not really an answer, just thoughts 😁

castastrophe

Nice updates and I appreciate the linting fixes introduced here too!

jawinn · 2024-03-26T15:26:57Z

It looks like the docs build is not including the Checkbox CSS on the page. It is including the Quick Actions CSS, that wraps the Checkbox:

castastrophe · 2024-03-26T15:49:03Z

It looks like the docs build is not including the Checkbox CSS on the page. It is including the Quick Actions CSS, that wraps the Checkbox:

This appears to be a bug on main so it doesn't necessarily have to block this PR. I can work on a patch for main that fixes this issue.

castastrophe · 2024-03-26T21:21:40Z

@jawinn I've got a fix going in here - #2611

fix(card): rename misnamed instance of background color mod The custom mod property for the background color was named incorrectly in one of the declarations. refactor(card): cleanup mods and vars around use of preview bg The custom properties and mods were not differentiating between the card background and the preview div background used in the quiet, gallery, and horizontal variations. This did not work well with the horizontal variant which has both a card background and a preview background, when some of the mis-named mods were fixed. A few unset custom properties with spectrum prefixed naming also appeared to be intended as mods, and now have mod equivalents: --spectrum-card-background-color-quiet --spectrum-card-background-color-hover-quiet --spectrum-card-content-margin-top-quiet --spectrum-card-minimum-width-quiet - Creates new custom properties for the preview background colors, that were previously being set through changing a mod value. - Renames --spectrum-card-preview-border-width to --spectrum-card-preview-border-width-selected to match with its usage

jawinn commented Jan 17, 2024

View reviewed changes

jawinn marked this pull request as ready for review January 23, 2024 20:36

jawinn force-pushed the jawinn/fix-card-mod-rename-refactor branch from e1fe044 to cd7e341 Compare January 23, 2024 21:17

jawinn requested review from pfulton, mdt2 and jenndiaz January 23, 2024 21:19

mdt2 reviewed Jan 29, 2024

View reviewed changes

jawinn added the ready-for-review label Feb 9, 2024

castastrophe force-pushed the jawinn/fix-card-mod-rename-refactor branch from cd7e341 to 942c91c Compare March 26, 2024 13:25

castastrophe approved these changes Mar 26, 2024

View reviewed changes

jawinn added the run_vrt For use on PRs looking to kick off VRT label Mar 26, 2024

jawinn force-pushed the jawinn/fix-card-mod-rename-refactor branch from 942c91c to 9146756 Compare March 26, 2024 15:14

castastrophe force-pushed the jawinn/fix-card-mod-rename-refactor branch from 9146756 to bfa2411 Compare April 1, 2024 13:53

castastrophe added ready-to-merge and removed ready-for-review labels Apr 1, 2024

castastrophe force-pushed the jawinn/fix-card-mod-rename-refactor branch from bfa2411 to 30ef29b Compare April 1, 2024 14:05

castastrophe enabled auto-merge (squash) April 1, 2024 14:05

castastrophe merged commit 553895d into main Apr 1, 2024
14 of 22 checks passed

castastrophe deleted the jawinn/fix-card-mod-rename-refactor branch April 1, 2024 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(card): rename mod and refactor some usage of mods and vars #2444

refactor(card): rename mod and refactor some usage of mods and vars #2444

jawinn commented Jan 17, 2024 •

edited by mdt2

Loading

github-actions bot commented Jan 17, 2024 •

edited

Loading

card

jawinn Jan 17, 2024 •

edited

Loading

mdt2 Jan 29, 2024

jawinn Feb 7, 2024

github-actions bot commented Jan 29, 2024 •

edited

Loading

mdt2 left a comment

mdt2 Jan 29, 2024

jawinn Feb 7, 2024

mdt2 Jan 29, 2024

castastrophe left a comment

jawinn commented Mar 26, 2024

castastrophe commented Mar 26, 2024

castastrophe commented Mar 26, 2024

refactor(card): rename mod and refactor some usage of mods and vars #2444

refactor(card): rename mod and refactor some usage of mods and vars #2444

Conversation

jawinn commented Jan 17, 2024 • edited by mdt2 Loading

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

github-actions bot commented Jan 17, 2024 • edited Loading

File metrics

Summary

card

jawinn Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

mdt2 Jan 29, 2024

Choose a reason for hiding this comment

jawinn Feb 7, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024 • edited Loading

mdt2 left a comment

Choose a reason for hiding this comment

mdt2 Jan 29, 2024

Choose a reason for hiding this comment

jawinn Feb 7, 2024

Choose a reason for hiding this comment

mdt2 Jan 29, 2024

Choose a reason for hiding this comment

castastrophe left a comment

Choose a reason for hiding this comment

jawinn commented Mar 26, 2024

castastrophe commented Mar 26, 2024

castastrophe commented Mar 26, 2024

jawinn commented Jan 17, 2024 •

edited by mdt2

Loading

github-actions bot commented Jan 17, 2024 •

edited

Loading

jawinn Jan 17, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading