-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(illustratedmessage): adding new s2 tokens #3246
base: spectrum-two
Are you sure you want to change the base?
feat(illustratedmessage): adding new s2 tokens #3246
Conversation
🦋 Changeset detectedLatest commit: 5ffcf6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
File metricsSummaryTotal size: 4.30 MB* Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.
Detailsdropzone
illustratedmessage
* Results are not gzipped or minified. * An ASCII character in UTF-8 is 8 bits or 1 byte. |
🚀 Deployed on https://pr-3246--spectrum-css.netlify.app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've got sizing and variants in here now, which is really exciting! Leaving some feedback to improve the custom property definitions and use of tokens!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great! There are some typography tokens that need to be updated to match the spec, and the illustration color token also needs updating, but otherwise colors and layouts are looking good!
A couple of other thoughts I had:
- Could we drop the update tokens commit and use the
yarn.lock
fromspectrum-two
? That might be safer than the updates on this branch. - We'll need to update documentation (like the Docs page in Storybook) for Illustrated message since we're introducing a new variant and t-shirt sizing), I think it'd be ok to do this later once we get these changes in
main
since we've had a lot of documentation changes between these two branches, but maybe we could ensure that we write a Jira card so we don't lose track of the fact that the work needs to be done? What do you think?
${when(hasButtons, () => | ||
ButtonGroup({ | ||
size, | ||
items: [ | ||
{ | ||
variant: "secondary", | ||
treatment: "outline", | ||
label: "Remind me later", | ||
|
||
}, | ||
{ | ||
variant: "accent", | ||
treatment: "fill", | ||
label: "Rate now", | ||
}, | ||
] | ||
}) | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re: the React hooks problem you're getting from adding this button group, I threw your template/stories code for hasButtons
quickly into the main branch and tested that control there (see the gif below), I don't think it's going to be an issue once this code gets to main, so no fixes needed on that! 🎉
It's the same issue that @marissahuysentruyt was having with the Has footer control on #2860
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the hooks error must be something on this branch. Cassondra fixed it with some of the major updates on main
, but spectrum-two
doesn't have those yet. I get the same thing on dialog 😢
Speaking of the button group, when we don't have the buttons, do we need to remove the margin that's on the description above it? This was just something I noticed, and I'm not sure about. 🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I could do is change it to margin-top-start
and add to the button group container
I thought I merged the updates from |
You did! There were just deviations in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think most of the tokens are good now! I left some comments about some things that still need to be cleaned up, let me know if you want to pair through any of these!
&:lang(ja), | ||
&:lang(zh), | ||
&:lang(ko) { | ||
--spectrum-illustrated-message-cjk-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see what you're trying to do here and I think this is a good strategy, I was originally thinking we should have something like:
&:lang(ja),
&:lang(zh),
&:lang(ko) {
--spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size);
}
and then also
.spectrum-IllustratedMessage--sizeS {
&:lang(ja),
&:lang(zh),
&:lang(ko) {
--spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-small-cjk-title-font-size);
}
}
.spectrum-IllustratedMessage--sizeL {
&:lang(ja),
&:lang(zh),
&:lang(ko) {
--spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-large-cjk-title-font-size);
}
}
That gets kind of ugly and repetitive since it needs to change for every t-shirt size though, so let's not do that!
Instead let's take what you already have, I think this has the potential to be cleaner, it's just that --spectrum-illustrated-message-cjk-title-font-size
isn't being used right now. What if we add a custom property definition to the title section like this:
--spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-medium-title-font-size);
--spectrum-illustrated-message-title-line-height: var(--spectrum-title-line-height);
--spectrum-illustrated-message-title-color: var(--spectrum-heading-color);
/* These first 3 are already in the code, I'm just adding them so you have an idea of where in the code this would be */
--spectrum-illustrated-message-cjk-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size);
and then change what you have here to
&:lang(ja),
&:lang(zh),
&:lang(ko) {
--spectrum-illustrated-message-title-font-size: var(--spectrum-illustrated-message-cjk-title-font-size);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cjk font sizes are not coming through and this custom property is showing up as unused in my editor:
You'll need to set --spectrum-illustrated-message-title-font-size
to --spectrum-illustrated-message-cjk-title-font-size
somewhere within the cjk lang selector. This --spectrum-illustrated-message-cjk-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size);
that you have currently can be moved up to around L33 where we have the other title custom properties set. Although I think it would also work if it stayed here within the cjk lang selector.
Adobe Clean doesn't render correctly for the component anymore 😅 |
Do you have a screen shot of what this looks like for you? I did notice some weirdness with tokens disappearing - like for instance the space between the description and button group would suddenly stop being defined, but I couldn't figure out what was triggering it and so far it's been rendering fine for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along! I left some more specific comments, but another more general concern I have is what regressions this might be causing to Drop Zone. Since VRTs aren't working great for spectrum-two
, I visually compared with another branch on spectrum-two
and see some differences that need looking into, primarily with the image size and typography, we'll probably want to look into why these are happening and if there's anything we can do to prevent them:
| `--mod-illustrated-message-heading-to-description` | | ||
| `--mod-illustrated-message-illustration-accent-color` | | ||
| `--mod-illustrated-message-illustration-color` | | ||
| `--mod-illustrated-message-illustration-size` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is also not noted in the changeset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the same thing that --mod-illustrated-message-illustration-size
isn't noted in the changeset lists.
It looks like --mod-illustrated-message-description-max-inline-size
, --mod-illustrated-message-heading-max-inline-size
, got removed or renamed. Should we note them in the changeset as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! This is starting to look really good and I think after another round of (smaller) changes, you could see if someone else wants to review it and also see if we can get illustrated message design reviewed.
A lot of the callouts I have now are more general and maybe up for discussion, so I'm not requesting changes this time around:
- Storybook controls: Comparing this branch with
main
, it looks like the accent color story disappears inmain
and uses the controls to flip the accent color on and off. It'd be great if we can make it like that in this branch, too, if possible! There's also a heading control that appears to control the heading text onmain
, so that might be another change to adopt here. - The illustrated message image: The Figma specs call for a square svg, and we're currently using a rectangular image. I would want to confirm with design that this always-square image might be something new for S2, since the way we have the CSS right now, the width and height use the same custom properties. If images are always meant to be square and never rectangular, we might want to update the image.
- Font size for cjk: This still needs some kind of fix to make sure the title cjk font size is being applied, but is probably worth asking design about what cjk tokens should be used other than the title font-size.
- Drop zone: There's still some wonkiness here! But I'm realizing now that I think this might be coming from token changes on
spectrum-two
and maybe it would be better to leave it alone until it's ready for migration 🥴
I also did occasionally have the problem I was noticing before with tokens, I'm attaching some screenshots at the bottom but basically what I'm seeing is that all of the typography styles are suddenly overriding the component-level styles, I don't think this is something that you did but might be worth investigating as a separate bug:
Will re-review when I'm back from PTO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the CJK tokens are the only ones I'm seeing that aren't coming through properly, and while I called it out last time, I'm actually seeing why that's happening now.
I'm also really bothered by the typography bug I'm seeing but would love to see another reviewer's opinion/know if they're seeing the same thing.
Last thing: since we have no package.json changes in this PR, can we remove the changes that appear from yarn.lock in the PR? (Totally fine if individual commits show changes, as long as the changes from all commits don't have yarn.lock listed.)
&:lang(ja), | ||
&:lang(zh), | ||
&:lang(ko) { | ||
--spectrum-illustrated-message-cjk-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These cjk font sizes are not coming through and this custom property is showing up as unused in my editor:
You'll need to set --spectrum-illustrated-message-title-font-size
to --spectrum-illustrated-message-cjk-title-font-size
somewhere within the cjk lang selector. This --spectrum-illustrated-message-cjk-title-font-size: var(--spectrum-illustrated-message-medium-cjk-title-font-size);
that you have currently can be moved up to around L33 where we have the other title custom properties set. Although I think it would also work if it stayed here within the cjk lang selector.
heading, | ||
() => | ||
html`<h2 | ||
class="spectrum-Heading spectrum-Heading--regular ${rootClass}-heading" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really love to hear others' opinions on this, but given that the title and body are setting font-weight, font-style, font-family, and font-size, I'm not seeing any rationale for keeping .spectrum-Heading
and .spectrum-Body
classes in the template. I'm still seeing that issue I mentioned last time where the typography classes are inconsistently prioritized over the component-specific classes and I don't know why that's happening (maybe it has something to do with removing the t-shirt sized typography classes?), but removing the typography classes here would likely fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow --spectrum-illustrated-message-cjk-title-font-size
didn't appear as an unused property until I reset my VSCode 😅 It was rendering normally for me on my end. I ended up adding the language selectors within each t-shirt size. For some reason moving that line of code more up top still comes up as unused property. Switching the languages look normal now.
For the .spectrum-Heading
and .spectrum-Body
the only reason why I kept it there was because main
branch had it. Removing it now doesn't break anything so it's good now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--spectrum-illustrated-message-heading-to-description: var(--spectrum-spacing-75); | ||
--spectrum-illustrated-message-illustration-to-heading: var(--spectrum-spacing-200); | ||
--spectrum-illustrated-message-illustration-to-content: var(--spectrum-spacing-200); | ||
--spectrum-illustration-message-description-to-action: var(--spectrum-spacing-300); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be --spectrum-illustrated-message-description-to-action
? I noticed the "illustration" vs "illustrated" difference on the PR description and changelog.
block-size: var(--mod-illustrated-message-illustration-size, var(--spectrum-illustrated-message-illustration-size)); | ||
inline-size: var(--mod-illustrated-message-illustration-size, var(--spectrum-illustrated-message-illustration-size)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine that someone customizing this will at some point will want to use an image that isn't a square aspect ratio. Should we provide mods to set the width and height separately? Perhaps this could be in addition to what you have: block-size: var(--mod-illustrated-message-illustration-block-size, var(--mod-illustrated-message-illustration-size, var(--spectrum-illustrated-message-illustration-size)));
--- the Icon component CSS is doing something similar.
@@ -36,9 +36,42 @@ export default { | |||
disable: true, | |||
}, | |||
}, | |||
isHorizontal: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a Docs story for the horizontal variant, along with any docs related to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holy smokes this was a big one! You covered a lot of stuff and this seems like it's getting in better and better shape- well done! I left you some questions and comments, and then I had a couple more questions that I didn't really know where to put in the files. Hopefully I don't repeat too much of what Rise & Josh have already mentioned.
Do we need an updated illustration at all? The designs are using that cloud, which seems very S2 ☁️😄 We should be able to export it directly from Figma (I can try to help with that if you want/need)
Do we want or need to allow users to test out the description text with a description
text control? I see it's disabled now, but I don't have the context as to why.
/** | ||
* An accent color class can be used on elements of the illustration SVG. | ||
*/ | ||
export const AccentColor = Template.bind({}); | ||
AccentColor.tags = ["!dev"]; | ||
AccentColor.args = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to ask about the sizing stories, but it looks like you and Rise already talked about those stories being added later. Do you want to make a card for the follow up work so we keep those additions on our radar once we merge? If you already created it, my apologies- I must have missed it in my very quick search! We could add the Figma design guidance around each of the sizes too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! I was also going to ask about the accent color story, but I see Rise's covered it! Just to clarify for me, design was ok with us keeping this story then in our documentation and examples for this component? I know you mentioned the accent color is used for the illustration and hover state in Drop zone, but should we continue showing that accent color variant here for illustrated message?
--spectrum-illustrated-message-illustration-size: 96px; | ||
|
||
/* Title */ | ||
--spectrum-illustrated-message-title-font-family: var(--spectrum-title-sans-serif-font-family); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to check with Rise & Josh on this one, but I would suggest we keep the original token here, and continue to use spectrum-sans-font-family-stack
instead of spectrum-title-sans-serif-font-family
.
Currently, this is what is in the tokens/dist
--spectrum-title-sans-serif-font-family:var(--spectrum-sans-serif-font-family);
...
--spectrum-sans-serif-font-family:Adobe Clean;
The title-sans-serif-font-family token (although called for in the design) isn't pointing to the web font stack, so we probably shouldn't be using that title-sans-serif token as it is currently. The correct web font stack is actually defined in that original sans-font-family-stack:
--spectrum-sans-font-family-stack: adobe-clean, var(--spectrum-sans-serif-font-family), 'Source Sans Pro', -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, Ubuntu, 'Trebuchet MS', 'Lucida Grande', sans-serif;
Josh mentioned that the font loaded by Typekit is the "adobe-clean", and which is only defined in our stack (not the "Adobe Clean"). I'll have to do the same thing for dialog, now that we've dug into it more.
--spectrum-illustrated-message-title-color: var(--spectrum-heading-color); | ||
|
||
/* Description */ | ||
--spectrum-illustrated-message-description-font-family: var(--spectrum-body-sans-serif-font-family); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might revert this change too. I think it's in a similar situation to that title token I mentioned, with "Adobe Clean" vs. "adobe-clean".
spectrum-css/tokens/dist/index.css
Line 1664 in 0c93977
--spectrum-body-sans-serif-font-family:var(--spectrum-sans-serif-font-family); |
`--mod-illustrated-message-illustration-accent-color` | ||
`--highcontrast-illustrated-message-illustration-accent-color` | ||
|
||
## Dropzone & Illustrated message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might just be me, but I wonder if we could come up with a clearer heading for this section. Maybe something like, "Dropzone nested mods?" Because those mods were added to the nested illustrated message within dropzone, right?
I don't know...take it or leave it.
@@ -9,6 +9,7 @@ | |||
| `--mod-drop-zone-body-font-style` | | |||
| `--mod-drop-zone-body-font-weight` | | |||
| `--mod-drop-zone-body-line-height` | | |||
| `--mod-drop-zone-body-to-action` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be added to the changeset message as "new mods" for drop zone!
--mod-drop-zone-body-to-action
--mod-drop-zone-illustration-to-title
max-inline-size: var(--mod-illustrated-message-description-max-inline-size, var(--spectrum-illustrated-message-description-max-inline-size)); | ||
margin-block-start: var(--mod-illustrated-message-heading-to-description, var(--spectrum-illustrated-message-heading-to-description)); | ||
margin-block-end: 0; | ||
.spectrum-IllustratedMessage-illustration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The block-size and inline-size tokens look good to me for the illlustration. But when I inspect the illustration in the browser in the horizontal orientation (or use the little measurement tool), it looks like the width is off in comparison to the vertical orientation. Were you seeing this at all? Maybe I'm incorrect in thinking they were supposed to be the same size regardless of orientation?
This is a large, horizontal illustrated message. I believe the small and medium are good 👍 I was expecting the large to also be 160x160.
| `--mod-illustrated-message-heading-to-description` | | ||
| `--mod-illustrated-message-illustration-accent-color` | | ||
| `--mod-illustrated-message-illustration-color` | | ||
| `--mod-illustrated-message-illustration-size` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the same thing that --mod-illustrated-message-illustration-size
isn't noted in the changeset lists.
It looks like --mod-illustrated-message-description-max-inline-size
, --mod-illustrated-message-heading-max-inline-size
, got removed or renamed. Should we note them in the changeset as well?
${when(hasButtons, () => | ||
ButtonGroup({ | ||
size, | ||
items: [ | ||
{ | ||
variant: "secondary", | ||
treatment: "outline", | ||
label: "Remind me later", | ||
|
||
}, | ||
{ | ||
variant: "accent", | ||
treatment: "fill", | ||
label: "Rate now", | ||
}, | ||
] | ||
}) | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the hooks error must be something on this branch. Cassondra fixed it with some of the major updates on main
, but spectrum-two
doesn't have those yet. I get the same thing on dialog 😢
Speaking of the button group, when we don't have the buttons, do we need to remove the margin that's on the description above it? This was just something I noticed, and I'm not sure about. 🤷♀️
Description
Adding new S2 tokens for
illustratedmessage
Illustrated Message
New tokens
--spectrum-illustrated-message-heading-to-description
--spectrum-illustrated-message-illustration-to-heading
--spectrum-illustrated-message-illustration-to-content
--spectrum-illustration-message-description-to-action
--spectrum-illustrated-message-illustration-color
--spectrum-illustrated-message-illustration-size
--spectrum-illustrated-message-small-cjk-title-font-size
--spectrum-illustrated-message-large-cjk-title-font-size
--spectrum-illustrated-message-large-body-font-size
--spectrum-illustrated-message-horizontal-maximum-width
New mods
--mod-illustrated-message-description-to-action
--mod-illustrated-message-heading-to-description
--mod-illustrated-message-illustration-to-content
--mod-illustrated-message-horizontal-maximum-width
Removed tokens
--spectrum-illustrated-message-title-to-heading
--spectrum-illustrated-message-description-max-inline-size
--spectrum-illustrated-message-heading-max-inline-size
--spectrum-illustrated-message-illustration-accent-color
--mod-illustrated-message-illustration-accent-color
--highcontrast-illustrated-message-illustration-accent-color
Dropzone
Renamed mods
--mod-illustrated-message-title-to-heading
>--mod-illustrated-message-illustration-to-heading
--mod-illustrated-message-heading-to-body
>--mod-illustrated-message-heading-to-description
--mod-illustrated-message-content-maximum-width
>--mod-illustrated-message-vertical-maximum-width
Validation steps
Regression testing
Validate:
Screenshots
To-do list