-
Notifications
You must be signed in to change notification settings - Fork 24
feat: Add children prop to <Banner> component to pass extra content
#985
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
children prop to <Banner> componentchildren prop to <Banner> component to pass extra content
| ) : null} | ||
| <Box display="flex" flexDirection="column" gap="small" flexGrow={1}> | ||
| <Box | ||
| className={styles.topContent} |
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.
The only reason you need this class is for:
.topContent {
flex-direction: column;
align-items: stretch;
}The first one, you can pass as a Box prop. The 2nd one, we do not support stretch yet on the alignItems prop. But maybe we should. Could you consider sneaking that change in this very PR, and then take advantage of it?
Then you'd be able to remove that topContent css class name, and do this here instead:
<Box
- className={styles.topContent}
+ flexDirection="column"
+ alignItems="stretch"If now, I can do it later in a new PR.
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.
These styles that you're referring to need to be applied only if @container banner (width < 235px) (very very small view ports):
@container banner (width < 235px) {
// ....
.topContent {
flex-direction: column;
align-items: stretch;
}
// ....
}to ensure that things "look good" even in that case:
If I were to apply flexDirection and alignItems to the <Box> regardless of the view port width (which I think it's what you're saying), then it would look weird when the view port is regular/wide:
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.
Oh, I missed that. In that case, forget what I said.
Short description
Background
The current
<Banner>can accept title, description, action..., but no "extra content".This PR
This PR a new
childrenprop to our existing<Banner>component.Usage:
Usage without children still supported of course (children prop is optional):
Reason
This is needed to support the new "upgrade banner" (with upgrade icon, a primary CTA and a list of enticing features) that we'll have to show in todoist-web in the Pro Legacy subscription tab:
and in the free subscription tab as well:
Reference
Test Plan
Demo