Skip to content

Conversation

@engfragui
Copy link
Contributor

@engfragui engfragui commented Nov 12, 2025

Short description

Background

The current <Banner> can accept title, description, action..., but no "extra content".

Screenshot 2025-11-13 at 07 47 41

This PR

This PR a new children prop to our existing <Banner> component.

Usage:

<Banner
    type="neutral"
    title="title"
    description="description"
    // other props...
>
    <div>Extra content here as children</div>
</Banner>

Usage without children still supported of course (children prop is optional):

<Banner
    type="neutral"
    title="title"
    description="description"
    // other props...
/>

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:

513439578-5888e62b-878e-419f-b1e4-a0951196bf82

and in the free subscription tab as well:

513439617-3929dfbd-8b98-44f1-b16d-572d30cf9a75

Reference

Test Plan

Demo

BeforeAfter
Screenshot 2025-11-13 at 07 47 41 Screenshot 2025-11-13 at 07 47 21

@engfragui engfragui self-assigned this Nov 12, 2025
@engfragui engfragui marked this pull request as draft November 12, 2025 17:15
@engfragui engfragui requested a review from doistbot November 12, 2025 18:00
@engfragui engfragui changed the title feat: Add children prop to <Banner> component feat: Add children prop to <Banner> component to pass extra content Nov 13, 2025
@engfragui engfragui requested review from gnapse and rfgamaral and removed request for doistbot November 13, 2025 06:56
@engfragui engfragui marked this pull request as ready for review November 13, 2025 10:23
) : null}
<Box display="flex" flexDirection="column" gap="small" flexGrow={1}>
<Box
className={styles.topContent}
Copy link
Contributor

@gnapse gnapse Nov 13, 2025

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.

Copy link
Contributor Author

@engfragui engfragui Nov 13, 2025

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:

Screenshot 2025-11-13 at 15 06 30

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:

Screenshot 2025-11-13 at 15 08 58

Copy link
Contributor

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.

@engfragui engfragui merged commit bed2ebc into main Nov 13, 2025
14 of 16 checks passed
@engfragui engfragui deleted the francesca/banner-slot branch November 13, 2025 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants