-
Notifications
You must be signed in to change notification settings - Fork 206
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
castastrophe
left a comment
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
customStyleson 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
globalsdata 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: falseto 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
marissahuysentruyt
left a comment
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. |
aramos-adobe
left a comment
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-819S2 migration for contextual help
This migrates the
contextual helpcomponent 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-familyNew 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-colorRemoved mods
--highcontrast-contextual-help-heading-colorScreenshots
To-do list