-
Notifications
You must be signed in to change notification settings - Fork 201
feat(contextual-help): S2 migration #3909
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
Conversation
🦋 Changeset detectedLatest commit: 52e6d7a The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
🚀 Deployed on https://pr-3909--spectrum-css.netlify.app |
File metricsSummaryTotal size: 1.42 MB*
contextualhelp
* An ASCII character in UTF-8 is 8 bits or 1 byte. |
3bdff09
to
1850227
Compare
de08037
to
57323f8
Compare
c56c432
to
f237622
Compare
f237622
to
40e2b6b
Compare
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 looking great. Well documented and thanks for the storybook clean-up. I left one small suggestion about how we document high contrast hooks.
1bceda1
to
75a08f1
Compare
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.
Some questions for you!
-
Is there a reason we have
customStyles
on every story? Can we get rid of any and use our CSS instead? I haven't actually tried removing them, so we might, I'm not sure. -
I couldn't comment on the action button code, but I think we need to gather the
globals
data so that we can appropriately render both the small action button for large/mobile, and the XS action button for medium/desktop
// before the return statement
const { globals = {} } = context;
const scale = globals.scale ?? "medium"; // this const may not be necessary, so you could refactor this out if you wanted
// then in the ActionButton call, change the size declaration:
size: scale === "medium" ? "xs" : "s",
- Could you double check all of the mods in the CSS? There were quite a few places where I was seeing
--mod-spectrum-contextual-help*
instead of just--mod-contextual-help*
. (places where it was already like that, I mean) - How does contextual help get triggered? Would we want to add
isSelected: false
to the action button at all, just to match the design, or is this component triggered because an action button was selected?
6bff760
to
662f926
Compare
f0df0a6
to
be5d959
Compare
277e230
to
a5fd388
Compare
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.
To me, this is looking really good! I'm going to approve, but I would suggest the custom prop removals Aziz mentioned. I didn't want to request changes again since he already caught them! WHCM looks good too- great work!
a5fd388
to
f73ec5c
Compare
📚 Branch previewPR #3909 has been deployed to Azure Blob Storage: https://spectrumcss.z13.web.core.windows.net/pr-3909/index.html. |
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.
Just one more token removal!
--spectrum-contextual-help-border-radius: var(--mod-popover-corner-radius);
I don't think we need this either since you're using --mod-popover-corner-radius
then you can remove the border-radius in the spectrum-ContextualHelp-popover
selector
f73ec5c
to
f059497
Compare
f059497
to
52e6d7a
Compare
52e6d7a
to
ef68364
Compare
Description
CSS-819
S2 migration for contextual help
This migrates the
contextual help
component to the latest Spectrum 2 designs. Custom properties have been remapped and added per the design specification.Typographic and color tokens have been updated per design specifications.
All existing popover positioning varations are supported.
New custom properties
--spectrum-contextual-help-body-color
--spectrum-contextual-help-body-line-height
--spectrum-contextual-help-body-sans-serif-font-family
--spectrum-contextual-help-body-sans-serif-font-style
--spectrum-contextual-help-body-sans-serif-font-weight
--spectrum-contextual-help-title-color
--spectrum-contextual-help-title-font-style
--spectrum-contextual-help-title-font-weight
--spectrum-contextual-help-title-line-height
--spectrum-contextual-help-title-sans-serif-font-family
New mods
--mod-contextual-help-body-line-height
--mod-contextual-help-body-sans-serif-font-family
--mod-contextual-help-body-sans-serif-font-style
--mod-contextual-help-body-sans-serif-font-weight
--mod-contextual-help-body-size
--mod-contextual-help-title-color
--mod-contextual-help-title-font-style
--mod-contextual-help-title-font-weight
--mod-contextual-help-title-line-height
--mod-contextual-help-title-sans-serif-font-family
--highcontrast-contextual-help-heading-color
--highcontrast-contextual-help-title-color
Removed mods
--highcontrast-contextual-help-heading-color
Screenshots
To-do list