feat: [announcement-bar] add dismissible support and accept node text - #930
rohanchkrabrty wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAnnouncementBar text now accepts ReactNode content. The component supports optional dismissal through a close button. Without an onDismiss callback, clicking the button hides the bar; with a callback, the component calls it and remains mounted. Tests and documentation cover these behaviors, the dismiss slot, and the button’s accessibility details. Sequence Diagram(s)sequenceDiagram
actor User
participant AnnouncementBar
participant DismissHandler as onDismiss callback
User->>AnnouncementBar: Click dismiss button
alt onDismiss is provided
AnnouncementBar->>DismissHandler: Call callback
Note over AnnouncementBar: Bar remains mounted
else onDismiss is omitted
AnnouncementBar->>AnnouncementBar: Set dismissed state and render nothing
end
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The dismissal feature appears ready to merge after normal checks; no actionable issue remains from this review. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The observed change is confined to UI rendering and dismissal. No privileged operation or security boundary crossing was identified, although use by downstream applications is not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation Issue [
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
| } | ||
|
|
||
| /* Out of the flex flow so the message stays centered in the bar. */ | ||
| .dismiss { |
There was a problem hiding this comment.
The X fits exactly inside the right padding, so long text runs right up to it with no gap. Maybe add a bit more right padding when dismissible is set.
| {leadingIcon} | ||
| </span> | ||
| )} | ||
| <Text |
There was a problem hiding this comment.
(suggestion)
Now that text can be any element, it's still wrapped in a <span>, so passing a <div> or <p> isn't valid HTML. Could you render it as a <div> instead? Text supports render:
<Text
render={<div />}
className={styles.text}
size='small'
weight='medium'
data-slot='announcement-bar-text'
>
{text}
</Text>
The layout stays the same since it's a flex item either way.
This branch was successfully deployed
2 participants
Summary
dismissibleandonDismiss, with the same behavior as Callout.dismissibleshows a close button. WithoutonDismiss, the bar hides itself. WithonDismiss, the bar stays mounted and the consumer removes it.aria-label="Dismiss announcement"and theannouncement-bar-dismissslot.textacceptsReactNodeinstead ofstring, so it can hold links or bold text.warning,success,info) are left out because Figma has only Normal, Error, and Gradient.dismissible,onDismiss, andonActionClickprops.Closes #596