Revisiting Banner's withinContentContainer context

[BPScott]

Context

<Banner> renders differently based upon if it is rendered within a "content container" - a <Card> or <Modal> component or not. It does this by consuming a context called withinContentContainer which both <Card> and <Modal> set. You can see this in action in Storybook by comparing the "default banner" and "banner in a card" stories.

While this is nice as it ensures the correct visual behaviour and "it just works™" in Cards and Modals it does go against the component design tenant of "define variations of components, don't style based on the container your component lives in".

Occasionally there are cases where it is useful to use the "within content" style of a banner not within a Card or Modal, which means that in these cases awkward hacks have to be used to create a context. This also occurs in Web's StoreSwitcher. I'm fairly sure that this will stop working in v4 as we move from the legacy to modern context API.

This then gets more complicated as there are two Card-style components in web that mock out the context too.

Having to rely on creating a context to control the layout of a component feels weird and I don't really like it. Alas I wasn't here when this was originally implemented so I'd love a bit more context on the thinking behind this decision at the time. I think @danrosenthal is the man with the answers I seek.

---

So here's some questions:

1. Do we want to expose this context as part of our public API and recommend to people that is the way to render "within content" banners?
2. If not then should we provide some other mechanism of opting into the "within content" style? A prop that defaults to false perhaps: <Banner inContent> (name up for debate)
3. If we do want to provide a prop to toggle this behaviour is it confusing to have a prop that only has an effect some of the time? e.g. <Card><Banner/></Card> and <Card><Banner inContent/></Card> would render identically.
4. If we don't want to expose the context as a public API (NO on 1) and we consider the prop sometimes having no effect confusing (YES on 3) then should we even have the context at all even as an internal mechanism? Dropping this would be a big breaking change and would mean we have to hunt for all Banners-inside-a-Card-or-Modal and add the prop to them as part of the v4 upgrade in web.

---

Personal reckons:

1. I dislike exposing the context as a public API.
2. I like adding a prop to allow people to opt into this in cases where they need the in-content style but not in a Card or Modal.
3. I don't think having a Banner prop that does nothing when used inside Card and Modal is overly confusing. It's not an ideal situation but I think this is better than making the big backwards-compat break of removing the context all together
4. I think the cost of fixing every Banner in consuming applications is a very large and I while I like ambition I don't have the appetite to take ownership for that at the moment.

---

Do people have any opinions on the above questions and any thoughts our current implementation or on alternative ways of solving this?

For more context, we started talking about this in slack

[ry5n]

I like the “principle of predictability” as a general rule, so I’m in favor of moving away from contextual styling.

I like the idea of adding a prop, and I don’t expect too much confusion around it. Keeping the internal context-based behavior for now also seems right, because it’s being considerate. With v4 we could choose to deprecate the internal context-based behavior (not sure how we would signal that to users, since it’s not part of any API), and then remove in v5?

[BPScott]

With v4 we could choose to deprecate the internal context-based behavior (not sure how we would signal that to users, since it’s not part of any API), and then remove in v5?

Oh now that is cunning. It feels very possible to keep the context, then do something like:

if (this.context.withinContentContainer && !this.props.inContent) {
 console.warn('You're rendering a banner within content, please add the inContent prop so things still work when we remove the context to make this work automatically');
}

With some particularly complex black-magic fuckery we might even be able to assert that through typings (but that might be a bit adventurous)

[danrosenthal]

In general I'm opposed to the approach this issue advocates for, and would prefer it remains a variant that is enabled through context.

My rationale for that is we want this styling to be consistently applied in the proper context. Allowing a prop to configure it means it could be applied inconsistently as the implementor sees fit. Using context, we know that banners will always have this variant applied in the right context, and never in the wrong context. It's a guarantee (or contract) of consistency.

I’m a little torn as it goes against the component design tenant of “define variations of components, don’t style based on the container your component lives in”.

@BPScott raised the above point, but I don't believe it applies in this case. The component is still responsible for its own styling, and is defining a variant. That variant is just being activated through an alternative means to passing props.

Regarding the point of this being challenging to apply due to how context works, and not wanting it to be a part of our public API, I think this problem goes away completely in our move to v4 and the new context API. Providing this context then becomes as trivial as importing a Provider component and wrapping children in it. That way, the variant continues to be applied in a consistent manner across banners.

[BPScott]

I'm happy to say that in V4 we keep the context as a way to render the new style. Allowing consumers to pick and choose when to use the inContent style has the potential to lead to inconsistant styling and makes it possible for mistakes to creep in.

Importing a publically available Provider and using that is a lot less hacky than how we currently have to fudge the legacy context. It makes sure styles are applied consistently, while still giving people an escape hatch to use the inContent style if needed. Adding a Provider is enough friction to make people think twice about throwing it in anywhere and potentially using the inContent style in a place it shouldn't be used.

Revised answers:

1. Yes, expose the context provider as part of our public API.
2. Don't add an inContent prop to Banner.
3. N/A
4. N/A

[BPScott]

We're keeping the current behaviour, and the migration to the modern context has already happened in #902

All that remains is to ensure the context provider is included in our public API

[AndrewMusgrave]

All that remains is to ensure the context provider is included in our public API

@BPScott I can polish this tomorrow, are we wanting to export it, or export it and document?

[BPScott]

I think just export it. Documenting it will just make people want to use it and 99% of the time they shouldn't need to

[charlesr1971]

Hi @BPScott

I am not sure whether this is relevant, but currently there is a problem with the Polaris Banner v12.0.0+:

https://community.shopify.com/c/shopify-cli-and-tools/polaris-banner-not-displaying-as-per-the-docs/m-p/2517386/highlight/true#M4370

It seems that by default, it displays as:

<div class="Polaris-Banner Polaris-Banner--withinContentContainer" tabindex="0" role="status" aria-live="polite">

And, the majority of us, would like to see the one, displayed in the official docs:

<div class="Polaris-Banner Polaris-Banner--withinPage" tabindex="0" role="status" aria-live="polite">

The --withinContentContainer version, looks quite frankly, awful, and I am sorry if this offends anyone, but thats the truth of the matter.
Plus, by default, we should see the Banner that is displayed, in the official docs:

https://polaris.shopify.com/components/feedback-indicators/banner

[sam-b-rose]

Hi @charlesr1971 👋

The Banner will display the default styling when it is not inside a Card component.

When used within a Card, the other style is used. This is automatically applied to help ensure consistency of banners styles depending on the context.

If you need the banner to use the default style, you can do so by rendering it outside components that are using the WithinContentContext provider: Card, LegacyCard, and Modal.

[charlesr1971]

@sam-b-rose Thanks for this information.

It might be an idea to add this, to the official Banner docs, at:

https://polaris.shopify.com/components/feedback-indicators/banner

This thread shows that there is quite a bit of confusion, out there in the Shopify Polaris community:

https://community.shopify.com/c/shopify-cli-and-tools/polaris-banner-not-displaying-as-per-the-docs/m-p/2395409#M4191

In fact, I managed to work out what was going on, as my last reply, in this thread, proves, but it took a few hours of inspecting CSS etc 😊

https://community.shopify.com/c/shopify-cli-and-tools/polaris-banner-not-displaying-as-per-the-docs/m-p/2517718/highlight/true#M4371

However, I feel that I shouldn't have needed to dig this deep, to find the answer.

This little paragraph doesn't quite cut the mustard:

It might be idea to show an image of both styles of the Banner, with a more detailed description?

[sam-b-rose]

It might be idea to show an image of both styles of the Banner, with a more detailed description?

Agreed, there is a lot of text content here which can hide some of the component guidance. I like your suggestion—having images or rendered examples inline with the guidance would be helpful.

I can create a new issue to improve this area of the documentation. Thanks @charlesr1971!

[charlesr1971]

No worries.
I know the community will appreciate it 🙂