feat(coachmark): S2 migration #3412

cdransf · 2024-11-21T19:37:10Z

Description

CSS-1024

S2 coachmark migration

This migrates the coachmark component to S2. Custom properties have been remapped per the design spec.

Before	After
--spectrum-heading-sans-serif-font-weight	--spectrum-title-sans-serif-font-weight
--spectrum-coach-mark-title-size	--spectrum-coach-mark-title-font-size
--spectrum-heading-line-height	--spectrum-title-line-height
--spectrum-coach-mark-body-size	--spectrum-coach-mark-body-font-size
--spectrum-body-sans-serif-font-style	--spectrum-body-serif-font-style
--spectrum-coach-mark-pagination-body-size	--spectrum-coach-mark-pagination-body-font-size

Additions

Adds --spectrum-coachmark-media-fixed-height for fixed 4:3 image variant and an accompanying --mod-coachmark-media-fixed-height mod. This variation has been added to the coachmark component story as a boolean control labeled as Image fixed height. The class is conditionally added within the hasImage block and, as such, will only impact rendering when hasImage is also true.

Validation steps

Open the URL for the PR.
Navigate to the coachmark component and verify no regressions have occurred.

Note: the new font-family tokens set the font to Adobe Clean but Adobe Clean is not loading on the branch created from the spectrum-two base branch.

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.

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

changeset-bot · 2024-11-21T19:37:14Z

🦋 Changeset detected

Latest commit: b1b6909

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/coachmark	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-11-21T19:40:03Z

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

github-actions · 2024-11-21T19:42:56Z

File metrics

Summary

Total size: 1.73 MB^*
Total change (Δ): 🔴 ⬆ 1.99 KB (0.11%)

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

Package	Size	Δ
coachmark	7.48 KB	🔴 ⬆ 1.99 KB

Details

coachmark

Filename	Head	Compared to base
index.css	7.48 KB	🔴 ⬆ 1.99 KB (36.14%)

* 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

I'm seeing a few things on the design not represented yet.

Line-height changes
I see body and pagination in the S2 specs marked as using line-height-200. I don't see the CSS using this token.

Keyboard shortcut
I don't see this new feature represented yet in our Docs examples or styles or controls. Does design need to define tokens for these keyboard shortcut elements on the specs?

4:3 and 16:19 image
The design shows an example of images at both of these aspect ratios, that we should demonstrate in our docs and maybe have a control for. I'm not exactly sure how this ties in with the sizing of the image and tokens.

Wait on popover to finalize?
This component is heavily reliant on popover, which has its S2 migration in progress and has been discussing and updating some items related to coachmark (#3365). So it may be worth waiting on that to merge before final review of its design.

components/coachmark/index.css

cdransf · 2024-11-22T19:41:42Z

I'm seeing a few things on the design not represented yet.

Line-height changes I see body and pagination in the S2 specs marked as using line-height-200. I don't see the CSS using this token.

Keyboard shortcut I don't see this new feature represented yet in our Docs examples or styles or controls. Does design need to define tokens for these keyboard shortcut elements on the specs?

4:3 and 16:19 image The design shows an example of images at both of these aspect ratios, that we should demonstrate in our docs and maybe have a control for. I'm not exactly sure how this ties in with the sizing of the image and tokens.

Wait on popover to finalize? This component is heavily reliant on popover, which has its S2 migration in progress and has been discussing and updating some items related to coachmark (#3365). So it may be worth waiting on that to merge before final review of its design.

Awesome catches! I updated the line-height and font tokens. I marked this blocked pending the popover work and started a thread about the image implementation (one token includes -minimum- which seems odd semantically.

cdransf · 2024-11-27T17:28:55Z

I'm seeing a few things on the design not represented yet.
Line-height changes I see body and pagination in the S2 specs marked as using line-height-200. I don't see the CSS using this token.
Keyboard shortcut I don't see this new feature represented yet in our Docs examples or styles or controls. Does design need to define tokens for these keyboard shortcut elements on the specs?
4:3 and 16:19 image The design shows an example of images at both of these aspect ratios, that we should demonstrate in our docs and maybe have a control for. I'm not exactly sure how this ties in with the sizing of the image and tokens.
Wait on popover to finalize? This component is heavily reliant on popover, which has its S2 migration in progress and has been discussing and updating some items related to coachmark (#3365). So it may be worth waiting on that to merge before final review of its design.

Awesome catches! I updated the line-height and font tokens. I marked this blocked pending the popover work and started a thread about the image implementation (one token includes -minimum- which seems odd semantically.

Spoke with design and it sounds like the minimum token is the minimum dimension for the coach mark image and 4:3 is not a strict max. This is the current state:

.spectrum-CoachMark-image-wrapper {
  block-size: var(--mod-coachmark-media-height, var(--spectrum-coachmark-media-height));
  min-block-size: var(--mod-coachmark-media-min-height, var(--spectrum-coachmark-media-min-height));
...
}

components/coachmark/index.css

jawinn

The line-height looks good, and thanks for finding out the intentions on the 4:3 and 16:9 examples (where 4:3 is now a variant class).

It looks like the use of the heights needs a little adjustments.
And the newly added keyboard shortcut feature needs to be built and documented.

The stories on the "Docs" page appear cut off, but that appears to be more popover related? That is possibly something that was already fixed in main as I don't see that issue there.

components/coachmark/stories/coachmark.stories.js

jawinn · 2024-12-19T20:09:56Z

components/coachmark/stories/coachmark.stories.js

I'd recommend documenting how images are displayed and the new fixed height class, by

adding a story for the Fixed height / 4:3 image, that includes the class name in the story description.

adding some description to the default image story to mention that the image has a minimum height but can grow to any height based on its dimensions

Marking this as unresolved as I don't see this in the stories/documentation yet. It's important to document because of the addition of a variant and for clarity around how the image sizing works. What I was suggesting was:

"With media" Docs story: include info in the description about how the image is sized/displayed here (image has a minimum height but can grow to any height based on its dimensions)

"With media, fixed image height" add a Docs story that shows and explains this and mentions the added class in the description

components/coachmark/index.css

components/coachmark/stories/coachmark.stories.js

marissahuysentruyt

I did a first pass and mainly focused on the stories and template files.

❓ Do you know why the coach indicator has a class name with --undefined in it? Is it a size? I'm unsure if it's coming from coach mark or popover (I think it's popover?) so maybe that's something I can address in the popover PR. It's like this on main as well, so this is just a curiosity.
❓ What would you think about renaming the WithMedia story to actual be Without media? Just based on Figma, it seems like the coach mark has a cover image by default. If that's true (and I would have to check, I don't actually know- that's just a guess based on the dev mode component), the default story and the with media story are the same right now. We'd also have to fix the sentence-casing of the story name and put that documentation elsewhere:

❓ Maybe we need to expedite those new keyboard command components. There's a few older cards for them (css-230 and css-231), so maybe we just need a coachmark follow up for when those are done? Would not having those block this currently? Just thinking out loud on this one...

I'll do a second pass for tokens & CSS tomorrow!

marissahuysentruyt · 2025-01-06T19:34:14Z

components/coachmark/stories/template.js

+				hasPagination,
+				() => html`
+					<div class="spectrum-CoachMark-step">
+						<bdo dir="ltr">${currentStep} of ${totalStepCount}</bdo>


whoooa I've never seen a <bdo> tag! Add it to my list of things to look up!

So this doesn't respond to RTL then? Do you know why that is? I only found this bit in the documentation, and I suppose I was expecting it to work like our actual pagination component (where it flips for RTL languages). In that screenshot it looks like to me the pagination for the coachmark flipped. What do you think? Maybe this is a question for design?

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/bdo

The <bdo> HTML element overrides the current directionality of text, so that the text within is rendered in a different direction.

Huh. I think keeping to what the pagination component does makes the most sense. This is the same pattern in a different component so I'd be inclined to go with what we're doing in the component dedicated to this behavior. But that's a very good question. 😅

components/coachmark/stories/coachmark.stories.js

marissahuysentruyt · 2025-01-06T21:31:14Z

components/coachmark/stories/coachmark.stories.js

+				category: "Content",
+			},
+			control: { type: "file", accept: ".svg,.png,.jpg,.jpeg,.webc" },
+		},
 	},
 	args: {
 		rootClass: "spectrum-CoachMark",
 		hasActionMenu: true,
 		hasPagination: true,
 		hasImage: false,


I was messing around in Figma and noticed that maybe some of these args have changed for the default coach mark now? The "default variant" doesn't have the action menu and does have the cover image. Should those args here be updated?

hasActionMenu: false, hasImage: true

And then we'd have to update the default args for the CoachContainer template too I think.

Would this then make the With Media example on the docs page unnecessary if the default has media?

@marissahuysentruyt @jawinn I updated the stories in this commit 0008d0. If we run with...

hasActionMenu: false, hasImage: true

...as default args, I've shuffled around the leading example to adhere to that state, the second in that block to show the action menu on and the media off. The 2nd block now shows the media variations added in the S2 migration.

marissahuysentruyt · 2025-01-06T22:01:29Z

components/coachmark/stories/coachmark.stories.js

 	},
 };

 export const Default = CoachMarkGroup.bind({});
 Default.tags = ["!autodocs"];
-Default.args = {};
+Default.args = {
+	image: "example-card-landscape.png",


Did we want to show the landscape image with the Standard story that's on the docs page as well? This Default isn't the only on the docs page, so right now, all the images are the portrait ones, and they get cut off because of the defined story height.

Standard.args = { image: "example-card-landscape.png", };

I think regardless, with the updates to the coachmark spacing, we should increase the story heights (maybe to 450ish?)

I updated the height(s) in 0008d0 while adjusting other aspects of the story(ies). ✨

components/coachmark/index.css

components/coachmark/stories/coachmark.stories.js

jawinn · 2025-01-06T21:44:09Z

components/coachmark/stories/coachmark.stories.js

Marking this as unresolved as I don't see this in the stories/documentation yet. It's important to document because of the addition of a variant and for clarity around how the image sizing works. What I was suggesting was:

"With media" Docs story: include info in the description about how the image is sized/displayed here (image has a minimum height but can grow to any height based on its dimensions)

"With media, fixed image height" add a Docs story that shows and explains this and mentions the added class in the description

components/coachmark/stories/coachmark.stories.js

jawinn · 2025-01-06T21:56:35Z

components/coachmark/index.css

I'm still looking for the keyboard shortcut feature that is part of the S2 design spec. Were you planning to add that (markup, new CSS, stories)?

I totally missed that! Will be digging into it shortly (and may have some questions on how best to approach it).

jawinn · 2025-01-06T22:34:00Z

components/coachmark/stories/coachmark.stories.js

Now that spectrum-two has the .test.js files for Chromatic, it'd be a good idea to make sure everything that we want covered by VRTs is added there. Things that come to mind are the fixed height media, and the regular media with a landscape image that is affected by its minimum height.

…efaults; update example heights; update default image

…xplainers

cdransf added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Nov 21, 2024

cdransf requested review from jawinn, rise-erpelding and marissahuysentruyt November 21, 2024 19:37

cdransf self-assigned this Nov 21, 2024

jawinn requested changes Nov 22, 2024

View reviewed changes

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

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

cdransf added the blocks Anything where Spectrum-CSS is blocking someone else label Nov 22, 2024

cdransf added blocked See description and comments for what is blocking this issue and removed blocks Anything where Spectrum-CSS is blocking someone else labels Nov 25, 2024

cdransf requested a review from jawinn December 3, 2024 00:01

castastrophe force-pushed the spectrum-two branch 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54

cdransf force-pushed the cdransf/s2-migration-coachmark branch 2 times, most recently from e9b9444 to 28878d9 Compare December 10, 2024 15:18

cdransf force-pushed the cdransf/s2-migration-coachmark branch from 28878d9 to 63bbe7a Compare December 17, 2024 14:24

github-actions bot reviewed Dec 17, 2024

View reviewed changes

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

cdransf removed the blocked See description and comments for what is blocking this issue label Dec 17, 2024

cdransf force-pushed the cdransf/s2-migration-coachmark branch from fcc97e9 to db908ca Compare December 18, 2024 22:53

jawinn requested changes Dec 19, 2024

View reviewed changes

jawinn reviewed Dec 19, 2024

View reviewed changes

components/coachmark/stories/coachmark.stories.js Show resolved Hide resolved

cdransf force-pushed the cdransf/s2-migration-coachmark branch 2 times, most recently from 2c5d15c to 4715dab Compare December 19, 2024 23:00

castastrophe force-pushed the spectrum-two branch 4 times, most recently from fc916f1 to 9d7f088 Compare December 29, 2024 19:50

cdransf force-pushed the cdransf/s2-migration-coachmark branch 4 times, most recently from 45aaa69 to 441d81e Compare January 3, 2025 22:10

cdransf requested a review from jawinn January 3, 2025 22:25

cdransf force-pushed the cdransf/s2-migration-coachmark branch 2 times, most recently from f3dea08 to 9519ce5 Compare January 6, 2025 17:10

cdransf added 5 commits January 6, 2025 12:40

feat(coachmark): s2 migration

f0e70f8

feat(coachmark): line height token

cf74b1c

feat: add fixed height coachmark image variation

b2fdaf8

chore(coachmark): adds description to imageIsFixedHeight control

1dae947

feat(coachmark): image control in story

7630128

cdransf force-pushed the cdransf/s2-migration-coachmark branch from 9519ce5 to 7630128 Compare January 6, 2025 20:40

marissahuysentruyt requested changes Jan 6, 2025

View reviewed changes

cdransf added 3 commits January 6, 2025 14:24

fix(coachmark): image source control label

11b1705

fix(coachmark): fixed height label

7758162

fix(coachmark): conditionally show image-dependent controls

2e2014d

jawinn requested changes Jan 6, 2025

View reviewed changes

cdransf added 5 commits January 6, 2025 14:38

fix(coachmark): css syntax error

e7c09ab

chore(coachmark): adjust default args

8a83806

chore(coachmark): update template and tests to handle updated story d…

6bfdff7

…efaults; update example heights; update default image

chore(coachmark): update arg names

b41d73e

chore(coachmark): refactor stories to display variations and update e…

b1b6909

…xplainers

cdransf force-pushed the cdransf/s2-migration-coachmark branch from 0008d0a to b1b6909 Compare January 6, 2025 23:50

cdransf added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. and removed size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. labels Jan 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(coachmark): S2 migration #3412

feat(coachmark): S2 migration #3412

cdransf commented Nov 21, 2024 •

edited

Loading

changeset-bot bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

coachmark

jawinn left a comment

cdransf commented Nov 22, 2024

cdransf commented Nov 27, 2024

jawinn left a comment

jawinn Dec 19, 2024 •

edited

Loading

jawinn Jan 6, 2025

marissahuysentruyt left a comment

marissahuysentruyt Jan 6, 2025

cdransf Jan 7, 2025

marissahuysentruyt Jan 6, 2025

cdransf Jan 6, 2025

cdransf Jan 6, 2025

marissahuysentruyt Jan 6, 2025

cdransf Jan 7, 2025

jawinn Jan 6, 2025

jawinn Jan 6, 2025

cdransf Jan 7, 2025

jawinn Jan 6, 2025

feat(coachmark): S2 migration #3412

Are you sure you want to change the base?

feat(coachmark): S2 migration #3412

Conversation

cdransf commented Nov 21, 2024 • edited Loading

Description

S2 coachmark migration

Additions

Validation steps

Regression testing

To-do list

changeset-bot bot commented Nov 21, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Nov 21, 2024 • edited Loading

github-actions bot commented Nov 21, 2024 • edited Loading

File metrics

Summary

coachmark

jawinn left a comment

Choose a reason for hiding this comment

cdransf commented Nov 22, 2024

cdransf commented Nov 27, 2024

jawinn left a comment

Choose a reason for hiding this comment

jawinn Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marissahuysentruyt 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

cdransf commented Nov 21, 2024 •

edited

Loading

changeset-bot bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading

jawinn Dec 19, 2024 •

edited

Loading