Component: Banner

[Copons]

As proposed by @folletto, here's the new UpgradeBanner component, aiming to replace UpgradeNudge and all its custom variations.

Goals:

• Keep the default as our current default
• Allow for smaller sizes, as well as richer content (lists, images, buttons, prices).
• Tweaked styling for mobile

Testing

http://calypso.localhost:3000/devdocs/blocks/upgrade-banners

Status

• Button version
• Compact card version
• Default card version
• Responsive
• Text CTA
• Button CTA
• Lists
• Custom colors
• Custom icons
• Analytics tracking
• Dismiss button
• Plan and feature props
• Semantic colors
• Plan prices
• Plan icons

Screenshots

[matticbot]

Test live: https://calypso.live/devdocs/design/banner?branch=add/upgrade-banner-component

[folletto]

Design wise it's looking great so far! :)

Just a tiny addition: can we add these rules to handle a more evident hover state aligned with how we do things elsewhere (i.e. themes, reader):

.card.upgrade-banner:hover {
  transition: all 100ms ease-in-out;
}

.card.upgrade-banner.is-card-link:hover {
  box-shadow: 0 0 0 1px #87a6bc, 0 2px 4px #c8d7e1
}

I made the selector as CSS ones, but add them as you see fit in the SCSS.

Some smaller design details which I'm sure are already on your list, but just to be sure:

1. The one-line banner should be slightly shorter (~54px for the one line, 64-67px for the two lines). I'm not for pixel perfection here, but the one liner is meant to use less space, so it's better if we can tweak it to be shorter.
2. The two lines version needs some more spacing between the two lines (like a margin-top: 3px on the description).
3. The description div shouldn't show if there's no description (this should make the fix to the above easy)
4. The description should be $gray-dark too. We are starting to be more careful in accessibility (contrast).
5. As the bullet list text (not the checkmarks which are fine in light gray).
6. .upgrade-banner__action is a bit too far from the right chevron. A margin-right: -6px would do, or even just a 0 if we don't want to use negatives... or any other solution that you feel it works.

Ace work there! Thanks!

[aduth]

Do we need this if the rendered <Button /> will be an anchor element anyways? If we do, we should at the very least call on page instead of window.location directly.

[folletto]

Good point: I'd also raise the issue that if there's a URL to point to, the link should always be a standard href= so it leverages of all the browser optimizations for it (open in a new tab, right click, drag'n'drop, etc).

[Copons]

Sorry guys, this wasn't supposed to be still here. I'm gonna remove it ASAP.

[aduth]

Had you considered using the Redux analytics action creators? I could even see this hooking into the onClick prop, something like:

export default connect( null, ( dispatch, { onClick, event } ) => {
	if ( event ) {
		onClick = flow( 
			onClick,
			partial( recordGoogleEvent, 'calypso_upgrade_banner_cta_click', { cta_name: event } )
		);
	}

	return { onClick };
} )( UpgradeBanner );

[Copons]

I pretty much copied it from the Card component without giving it much thought, and that's exactly why it's on the todo list. :)

[aduth]

Some docs for the component would be good, since it's not obvious to me why button wouldn't be an indicator for rendering the button (here, appears opposite?)

[Copons]

Readme incoming. I was more interested in pushing to trigger some design related discussions. :)

[aduth]

Should we check length (perhaps size( list ) > 0 using _.size) to avoid rendering the list if it's empty?

[Copons]

Considering that the purpose of this component is display some copy text, is it really something we should be worried about?
(Also, is there a reason to use _.size instead of Array.length?)

[aduth]

I'm just imagining some case where we dynamically compose a list to pass to this component, and if that list happens to be empty, we might render this element, which I assume would have some base styling applied to push margins etc?

(Also, is there a reason to use _.size instead of Array.length?)

const list = undefined;

console.log( size( list ) );
// => 0

console.log( list.length ) );
// Uncaught TypeError: Cannot read property 'length' of undefined

[aduth]

Usually I find if you're creating separate render methods, either (a) it should be inlined into the base render method or (b) it should be split into a separate component.

[Copons]

I guess a separate button component could make sense in this case, considering how many more props the card version has compared to the button one.
As the button version is pretty much ready, though, maybe I could include it here and split in a following PR?

[davewhitley]

Looping in @rickybanister

Ties into the discussion around extending the nudge to other uses than upgrading

[mtias]

The way it is implemented at the moment, this seems like a component and not a block to me. The main benefit of the old upgrade nudge is that it abstracted a few things for you. For instance, you would pass a feature prop and it would build a standard url highlighting a plan row.

It also handled display logic for showing or not showing a nudge based on what features your current plan supports, which is very important.

Finally, I suggest looking at the cosmetic props (like color) and removing them in favor of the component handling it. If a feature belongs to a specific plan, the component would use the color from said plan, etc.

Same with price, it'd be good for the block to grab the price via a selector, because there are many intricacies there that should not be left up to be figured out by the person using one of these banners.

[Copons]

@folletto

The one-line banner should be slightly shorter (~54px for the one line, 64-67px for the two lines).

With the new margin-top: 3px; for the description, the two versions have the following heights:

• Title only: 54px
• Title + description: 62px

Do you think it's fine like this, or should I increase the two-liners vertical paddings?

[folletto]

Do you think it's fine like this, or should I increase the two-liners vertical paddings?

Let's keep that. We can always tweak later. :)

[Copons]

I've added the Banner i3 version.
It makes the most of plans, with automatic colors and icons.

Colors: I've opted for the strictest approach.
Now the Banner is blue except when a plan is defined. In those cases, the Banner automatically takes the plan color (personal = yellow; premium = green; business = purple).
Custom colors are not allowed anymore.

Icons: icons are mandatory if no plan is defined. Otherwise, the icon defaults to the PlanIcon.

Testable at http://calypso.localhost:3000/devdocs/design/banner

[Copons]

i3 screenshot:

[folletto]

I think we are almost there! :)

The only odd thing in the DevDocs is that the gridicons-info-outline default icon seems off centered. It's adjusted with .banner__icon-circle { padding: 3px 4px 4px 3px; ... }... can you check if it works also with other icons, just to be sure?

I'd also suggest to have a canonical example of each of the three plans banners in the showcase: identical, just with the text changed to match the plan and the same structure. So it's clear what changes from one plan to the other, and just that change. I'd thus then add a separate one that changes the icon from the default. To be clearer:

1. Default compact banner (as it is now)
2. Default banner with second line and changed icon
3. "Upgrade to Personal Plan!"
4. "Upgrade to Premium Plan!"
5. "Upgrade to Business Plan!"
6. The banner with everything — price and button combination.
7. The banner with everything — close and button combination.

Makes sense?

[Copons]

@folletto It does, and yes, the icons seemed off to me too.
And not only the info-outline, which is more obvious since it's round as the background, but also the star (check the old screens) and the trophy, even though it's not as noticeable.

Gonna fix the icons and add the dismiss button asap!

[Copons]

New screenshot with all features added (cc @folletto)

All things considered, I stick with my proposal of having several sub-components (ie. DismissibleBanner, CtaBanner, etc. - the sky's the limit!) with limited options for ease of use, while still providing the main Banner for extremely customized uses.
Just see how many props and quirks there are in the README.

[folletto]

I did a test run, design wise 👍

[Copons]

At this point I'd say we should review and ship this now and, if needs arise, extend it with specific sub-components at a later time.

[aduth]

If there's no reasonable default, we don't need to assign a defaultProps value. We could just not define any default and it would work just as well, leveraging truthiness of undefined prop in the condition where it's used. Also saves you the need to assign it as a oneOf option in propTypes.

[aduth]

Minor notes, but otherwise LGTM 👍

[aduth]

You can drop the spread:

feature: PropTypes.oneOf( getValidFeatureKeys() ),

[Copons]

Dang, you're totally right!

[aduth]

ESLint does not warn yet, but static property and instance property initializers need semicolons.

https://github.com/tc39/proposal-class-public-fields/pull/45/files
https://tc39.github.io/proposal-class-public-fields/

[Copons]

Uh, I wasn't aware of this.
Gonna fix it and merge.