-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Notification] Add high contrast style #2354
Comments
Notifications need to get the same dropshadow that is used for a Dropdown drawer and Modal |
@laurenmrice: @emyarod is working on this change, but it requires functionality of our theming package that we don't currently support. Before we spend the time coming up with such functionality to have component-specific theme tokens that can't simply be our global theme tokens (draft PR 2405 and issue 517 linked above)... can you verify the intent to deviate from our global theme tokens for our notification component? Are there accessibility reasons for this deviation? Or what is the justification? |
We originally made a full color version of the notifications for the white theme (first screenshot). When we started transferring components into the Gray 10, Gray 90 and Gray 100 themes, the colors of the notifications were not working consistently across themes and we ran into many accessibility issues. We are now proposing that we stick with the high contrast notifications that IDL originally speced (second screenshot). The only issue we are running into now is that these notification background colors are inverse colors of the theme background that they sit on, which causes some of the color support tokens to not be accessible (one color step up or down). This is the only instance where we are having the problem. We did not want to make global tokens just for notifications. @mattrosno |
Lauren and I we misunderstanding the meaning of global and component level tokens. We actually want to add new global tokens in the interim, like we currently already have in place for notifications (see below) and other components. We just dont want them surfaced with the core set of color tokens. These tokens would only be for notifications. Except instead of background they need to be for the accent color on the bar and icon.
|
@aagonzales that code snippet is an example of component tokens. |
I thought @tw15egan said those are all "global" in the current implementation. |
Can a dev please recommend a course of action for fixing our notification then? What are our paths forward or are we stuck with the broken pastel colors? |
They are global in the sense that they can be modified globally and do not live inside the notification component itself. For now, Shixie has proposed just adding a white background behind the pastel tints so the pastel backgrounds do not break on dark theme until a permanent fix can be done. |
When would a permanent fix be available? V11? |
I don’t think it would be a breaking change, just need to figure out a way to scope variables. Sort of like what we had back in the day with light-ui |
Hey everyone, because this is a high priority fix, I'm jumping in with another suggestion (see img below right hand side). As we wait on the tokens to play out, what if we fix notification for the time being without changing the token values, and just give them all a white background so the yellow tint shows up properly + drop shadow so they pop on Gray 10. |
@shixiedesign yes! I recommend using a pseudo-element to layer behind the notification and set the background of the pseudo-element to white. I haven't looked at the source, but that's how I've handle notification transparencies in the past. |
You can use the sass The colors for our warning tags were derived by changing the transparent of IBM's yellow.
Breaks down to |
Adding a dropshadow does seem to be the only solution for fixing the pastel notification. However, for inline notifications they feel off to be the only thing floating, most of the time inline notifications don't cover anything up or live on top of something which is when you'd want a dropshadow. If this solution is only going to be for a couple of weeks then I'm fine with it but if its going to be the until V11 or later I'd like to consider another option. Other optionIf we added |
Well haha @mattrosno even I am loving this proposal and I keep stressing this is an interim solution! Like @aagonzales said, inline notification with drop shadow is not great but then G10 theme forces it to have it. Also @jeanservaas made the point to me that the pastel on light ui is a style that is inconsistent with when they are on dark ui. I think ultimately we want to push for high contrast notification, which will solve all these problems. 👉 Let's keep this issue open to track High Contrast notification work. Side note, there's also a chance that we can save this notification style, and just have both HC and Pastel as two different options. One with drop shadow, and HC does not. I'm just thinking out loud. |
Issue for adding inverse tokens to enable work on high contrast notifications: #2684 |
Designers made a change to notification component styles. The colored styles were not working across the different themes so we switched them to high contrast moments.
Old:
New:
The text was updated successfully, but these errors were encountered: