-
Notifications
You must be signed in to change notification settings - Fork 100
feat(notice): update styles for SHINE #2109
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
base: beta
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: bbaf531 The changes in this PR will be included in the next version bump. 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 |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| // TODO: decouple .s-notice--btn from .s-btn | ||
| button&--dismiss { |
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.
Introduced s-notice--dismiss class here to make the dismiss button more explicit and style it accordingly.
| /** | ||
| * Whether to include a dismiss button on the notice or not | ||
| */ | ||
| dismissable: boolean; | ||
| /** | ||
| * Optional dismiss event handler | ||
| */ | ||
| onDismiss?: MouseEventHandler<HTMLButtonElement>; | ||
| /** | ||
| * Dismiss button label override | ||
| */ | ||
| i18nDismissButtonLabel?: string; | ||
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 made the dismiss button a boolean flag here with additional props instead of a special NoticeAction case like it was before. I think this make more sense and improves usability. Hopefully other folks agree 🙏🏾
| align-items: center; | ||
| gap: var(--su4); | ||
|
|
||
| &--icon { |
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.
Introduced the s-notice--icon class for the icons
| <button type="button" class="s-link s-link__underlined">Undo</button> | ||
| <button type="button" class="s-link s-notice--dismiss js-toast-close" aria-label="Dismiss" data-action="s-toast#hide">{% icon "ClearSm" %}</button> |
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 opted to style the buttons with s-link but not sure if I should've used s-btn s-btn__link instead like we do in the Svelte component for Notices.
CGuindon
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.
Looking great, just noticing some weirdness when I resize the screen smaller:
- the whole browser starts to shrink instead of resize when I go below 600px width — feels like a docs issue but not sure if the toast is affecting this somehow
- Notices can wrap to multiple lines when needed:
- Padding right should maintain a minimum of 12px (even when there's no X or under button, the description text shouldn't touch the right edge of the notice)
- Links on the end shouldn't wrap within itself (keep as single line unless the whole description is wrapping)
- Code snipets shouldn't wrap within itself either
Added an extra example in the variations section to make that a bit clearer
For the toast not banner (possibly for the other ticket):
- I noticed when I add the
s-notice__importantto the toast example, the undo link and close icon stay black instead of turning white as well.

Summary
This PR updates the Notice components according to the latest SHINE designs.
Additionally, the changes in this PR gets the Toast component 95% there. However I will open a follow-up PR's for the Toast and Banner components as they are separate tickets.
How to Test
As mentioned, only test the Notice component in Stacks Docs and Stacks Svelte: