primer / brand

React components and Primitives for GitHub marketing websites
https://primer.style/brand
MIT License
74 stars 33 forks source link

[Modification request] CTA banner to support a muted/secondary variant #752

Closed nsolerieu closed 3 weeks ago

nsolerieu commented 2 months ago

Currently the CTA banner component supports only a primary background color. We often end up customizing the background via one-off CSS and noticed that we are often using the muted bg color similarly to card.

See example here: https://github.com/orgs/github/projects/19671/views/1?pane=issue&itemId=79615610

Only the border allow to visually delineate the component which is limiting and sometime looks very heavy in dense pages

Image

Urgency

We are going to use one-off CSS override for the newsroom press release so there isn't a critical need for a component update

joshfarrant commented 2 months ago

Thanks for opening this @nsolerieu 👋

I'd be keen to get @danielguillan's take on this

danielguillan commented 2 months ago

Yes, the CTA Banner component currently supports setting a background color using the --brand-CTABanner-bgColor custom property. We could consider adding a new backgroundColor property to align it with BreakoutBanner, which supports default, subtle, and custom background colors.

Should we make subtle the default background color as well? This change would make sense if we use it most often, but it could introduce a visual breaking change.

nsolerieu commented 2 months ago

I'm supportive of aligning with the BreakoutBanner to have default/subtle via backgroundColor prop

joshfarrant commented 2 months ago

It looks like this is sort of related to https://github.com/primer/brand/issues/756 too.

It might be worth looking at CTABanner backgrounds holistically and coming up with a solution that works for both background colour and background image. It seems like BreakoutBanner supports both, so we could just do the same as we've done there.

The only thing I'd be a bit hesitant of is setting a new default background-color for the CTABanner as it could just end up creating work (via a breaking visual change) with potentially little benefit.

nsolerieu commented 2 months ago

@joshfarrant I'm also in favor to bring parity between CTABanner and BreakoutBanner background options