feat(announcement): add <rh-announcement> v2
#2153
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Continuation on PR #1722.
🦋 Changeset detectedLatest commit: 0edd9e0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: +2.38 kB (+1.15%) Total Size: 209 kB
ℹ️ View Unchanged
|
3 tasks
|
Comments from my review of the demos. Thanks Adam for sharing! Spacing adjustmentsAnnouncement documentation spacing for reference:
Space between the image and paragraph text should be 32px on desktop instead of 24px.
I noticed that there is extra margin of 32px on the right hand side after the default CTA which slightly makes the content not centered. This could be removed at a desktop size but we will want to make sure that at all breakpoint sizes there is at least a 16px buffer between the CTA / paragraph text and the close button.
The 32px margin on the right really impacts the mobile size as seen below. We can free up at least 16px of space to give the content some more room to stretch out.
Maybe applying 16px to the close button instead of the end of the CTA would cover the needed right spacing of 16px at mobile sizes
Close button touch targetThis might not be an issue but wanted to inquire about the touch target size for the close button. Does it at minimum meet the required 24x24px target size? When I look at the focus size it shows 16x16px Just trying to meet our accessibility standards for a really small icon that has a big ol' functional purpose for users. |
marionnegp
requested changes
Feb 5, 2025
There was a problem hiding this comment.
-
Should be there be space between the content and the close button? (cc @dmazon-redhat)
-
I noticed that the dark color palettes all have the overriding
dark-altsurface color. Should the light color palettes all use thelightestsurface color?
|
Made some spacing adjustments. PTAL.
@dmazon-redhat let me know if this change needs made. TY! |
|
FYI docs are now up. PTAL. I have the personalization/implementation docs coded but stashed on my local computer while we confirm they are okay to make public. |
bennypowers
requested changes
Feb 9, 2025
| text-align: center; | ||
|
|
||
| &.dark { | ||
| --rh-color-surface: var(--rh-color-surface-dark-alt, #292929); |
There was a problem hiding this comment.
can we avoid dark-alt? it's kind of a footgun in our theming system
There was a problem hiding this comment.
This question is likely best answered by @dmazon-redhat. Is it possible to have announcement follow our theming system's colors as follows?
- Dark 👉
var(--rh-color-surface-dark, #383838) - Darker 👉
var(--rh-color-surface-darker, #1f1f1f) - Darkest 👉
var(--rh-color-surface-darkest, #151515)
See this video demonstrating what those colors look like in action.
Most (all?) of our components follow these conventions. If necessary, users can customize this via something like:
rh-announcement[color-palette="darker"] { --rh-color-surface: powderblue; }@bennypowers LMK if you have ideas/direction about this via color transformations (color-mix?).
There was a problem hiding this comment.
maybe something like this:
#container {
background: var(--rh-color-surface);
&.dark {
background: oklch(from var(--rh-color-surface) calc(l + 10%) c h);
}
}https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/oklch
…t-design-system into feat/rh-announcement-v2
…ntent color definitions
marionnegp
approved these changes
Feb 10, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I did
<rh-announcement>to a new PR because the old PR (feat(announcement): add<rh-announcement>#1722) had so many merge conflicts.<rh-announcement>. See the Figma file.imgleftboolean attribute toimage-position="inline-start".Testing Instructions
<rh-announcement>#1722inline-startandblock-startbodyslotNotes to Reviewers
Closes #1576.
To do's