Skip to content
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

Notice: Add new "Inline" variation with a more subdued design #22924

Closed
wants to merge 1 commit into from

Conversation

shaunandrews
Copy link
Contributor

@shaunandrews shaunandrews commented Mar 1, 2018

Sometimes the <Notice> component is used inline on a page. /plugins/ is a prime example:

screen shot 2018-03-01 at 4 04 48 pm

The problem here is that these notices tend to really stand out. Maybe thats the point, and they should be acted upon. But I thought I'd try to see if we could offer a more subdued variant of notices that better "fit" in these types of use cases:

screen shot 2018-03-01 at 4 04 25 pm

--

I add a new prop to <Notice> called isInline which toggles a class is-inline to add a more subdued variation of the notice design. Its currently only updated to be used on /plans/.

This should help fix #20017

`is-inline` to add a more subdued variation of the notice design.
@shaunandrews shaunandrews added [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Mar 1, 2018
@shaunandrews shaunandrews self-assigned this Mar 1, 2018
@matticbot
Copy link
Contributor

@folletto
Copy link
Contributor

folletto commented Mar 8, 2018

Feels better to me.

Aside (not related to this PR): I'm feeling Notices and InlineNotices should really be two components.

@davewhitley
Copy link
Contributor

I think notices in general are becoming convoluted and confused in their purpose, but the design here looks good, and it's better than it was before.

@MichaelArestad
Copy link
Contributor

I already expressed the following to @shaunandrews, but def worth elaborating on here:

I think it might be a bit complex to have an extra prop for this. It might be forgotten or used inconsistently unintentionally.

I do like the way they look inline. I wonder if all Notices should look similar with the lighter theme. The global notices, however could remain the dark theme. This would sort of separate them out and keep their purposes/visuals clearer for folks. Thoughts?

@rickybanister
Copy link

Are there places 'inline' where we would rather use the 'compact' notice (dark) style instead of the light style?

Or for conjecture, could you overwrite the compact notices with this style @shaunandrews to let us play around with that and see if it could be a permanent replacement?

I agree with @drw158 that notices are really being stretched in their purpose. So maybe it would be easiest to let global notices be the dark style (since it does really contrast from the page) and we use Shaun's style in the page inline. It seems to satisfy the accessibility concerns, I'd just like to confirm it looks alright in real world scenarios.

@davewhitley
Copy link
Contributor

davewhitley commented Mar 21, 2018

Personally, I've needed a light-style inline notice for things that do not need immediate attention (like a global notice).

In this example, it's a permanent notice, so I didn't feel like a dark notice would be appropriate.

download

I like the idea of inline notices defaulting to light, and global ones defaulting to dark, but with the option to change. I could see a case where someone might want to use a dark inline notice. One example of a dark inline notice, would be the domain upgrade notice in the sidebar.

@folletto
Copy link
Contributor

folletto commented Mar 21, 2018

I feel there's an overlap with terms... maybe not, but just to be sure I also did a cross-check on how the component structure is now.

  1. We have inline and global notices ('Notice' and 'GlobalNotices' components)
  2. Inline notices can be normal or compact.

I was mentioning above the split Notice and InlineNotice. Now that I checked the code, the component split is already there (Notice and GlobalNotices) so I think the issue isn't now componentization, but that GlobalNotices shows a list of Notice.

Which means that Notice is doing double duty as both inline and global.

So:

Personally, I've needed a light-style inline notice for things that do not need immediate attention, like a global notice.

In this example, it's a permanent notice, so I didn't feel like a dark notice would be appropriate.

Elaborating on this, I'd suggest this approach:

  • Notice (which are inline, both normal and compact) — always light, meant to be inline.
  • GlobalNotices (which are the floating bubbles at the top-right) — always dark, meant to bubble up.

I'd approach this by creating a new GlobalNotice (singular) that is basically a Notice clone, dark, to be used exclusively as floating in the GlobalNotices (plural, list) component.

Once this component split is done, we can evolve the two with their specific use case and intent in mind.

Thoughts?

@rickybanister
Copy link

That makes sense @folletto and is a more articulate proposal than I made.

@MichaelArestad
Copy link
Contributor

@drw158 I would suggest using the Banner component instead for things that are more like ads or less urgent CTAs.

@folletto
Copy link
Contributor

folletto commented Mar 21, 2018

I would suggest using the Banner component instead for things that are more like ads or less urgent CTAs.

Yep: Banner was created (ref: #9653) for promotions, ads and upsells. We should strictly use it only for that scenario, for which it's ideal (vs informative notices): even if the UI can be analogous in some scenarios with the current design, that's not intended to stay that way (and being a banner, I totally expect to get even more evident and glaring).

@davewhitley
Copy link
Contributor

@MichaelArestad I considered that, and that's what my mockup shows (with an adjustment to the icon), but I felt it wasn't a good solution because it looks different than the other notices (solid block of color of the left and slight differences to the type). Also, I really felt it should be an alert/notice and not a generic banner.

@MichaelArestad
Copy link
Contributor

because it looks different than the other notices (solid block of color of the left and slight differences to the type). Also, I really felt it should be an alert/notice and not a generic banner.

Are there other notices on that view that it conflicts with? It looks like the exact use case for Banner. We use it for similar things all the time, especially plan upgrade pitches.

@davewhitley
Copy link
Contributor

davewhitley commented Mar 21, 2018

@MichaelArestad there aren't any other notices or banners on that page that I'm aware of, but it's just a static mockup at this point.

It looks like the exact use case for Banner. We use it for similar things all the time, especially plan upgrade pitches.

I disagree on this point.

Offer valid in US after spending the first $25 on Google Adwords.

It could be worded better, but this is really an alert or informative notice, not an ad. US and the $25 are prerequisites for using the Google AdWords coupon, and the user should be aware of that (If I'm not mistaken). But it's not urgent, which is why I don't think a dark notice is appropriate.

Overall, I don't mind using the banner temporarily, but it's not an ideal solution, and I think this kind of problem will come up again, and we could do more to accommodate different needs.

@folletto
Copy link
Contributor

Just for the sake of this PR — I think the proposal above should solve the issue regardless of what's being decided at the end (the banner discussion on the specific case above can happen elsewhere). :)

@michaeldcain
Copy link
Member

I'd approach this by creating a new GlobalNotice (singular) that is basically a Notice clone, dark, to be used exclusively as floating in the GlobalNotices (plural, list) component.

@folletto: I think this is what you meant, but wanted to clarify. If style componentization is the goal, I'd recommend that GlobalNotice literally be Notice with an is-global class and its own stylesheet, rather than a clone of the functional aspects (which could be changed at a later point if need be).

@folletto
Copy link
Contributor

I think this is what you meant, but wanted to clarify. If style componentization is the goal, I'd recommend that GlobalNotice literally be Notice with an is-global class and its own stylesheet, rather than a clone of the functional aspects (which could be changed at a later point if need be).

I've not reviewed the code close enough to make a call on that. I think that as long as it's a separate component – thus allowing more clear intent – and it's maintainable, I think it hits the spot.

@alisterscott
Copy link
Contributor

This branch needs a rebase. Is this PR still required?

@matticbot matticbot removed [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Jul 26, 2018
@alisterscott alisterscott deleted the update/notice-inline-variation branch August 8, 2018 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notices: compact inline notices have too much contrast in certain areas
8 participants