fix(notification): change box-shadow to border

[asudoh]

This change also makes inline notification's text color non-themeable for now (good to have your thoughts here @aagonzales given it's different from your latest spec), given the background color is non-themeable and making it themeable requires themeable component-specific token (or new global theme tokens).

This stop-gap approach may be enough for short team because IIRC low-contrast notifications is there for a backword-compatibility reason. (CC @shixiedesign to verify my memory)

Refs #4282.

Changelog

Changed

• Replaced box shadow for inline notification with border.
• Made the text color of inline notification fixed.

Testing / Reviewing

Testing should make sure our inline notification is not broken.

[netlify]

Deploy preview for the-carbon-components ready!

Built with commit e6a76de

https://deploy-preview-4299--the-carbon-components.netlify.com

[netlify]

Deploy preview for carbon-elements ready!

Built with commit e6a76de

https://deploy-preview-4299--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit e6a76de

https://deploy-preview-4299--carbon-components-react.netlify.com

[emyarod]

just a few notes on the pseudo elements but looks good to me, dark theme low contrast notifications are readable again with stop gap solution, and drop shadows have been replaced by borders

[abbeyhrt]

@asudoh should this address the Toast variation as well? Noticed this:

Sorry if I'm totally off here or if this is being addressed elsewhere!

[asudoh]

Great catch @abbeyhrt! Fixed.

[abbeyhrt]

Descriptions still aren't showing up for me but the titles are!

[asudoh]

Oops another good catch @abbeyhrt! Fixed.

[aagonzales]

Yeah our notifications kind of break our theming modal (without component level tokens). I think its ok for now if we make these non-themeable. I think its more important that they look right for use at IBM than allowing people to theme them. I know there were issues brought up around this last time we tried to update notifications and I'm not sure where the discussion ended with that.

The Gray 10 and White low contrast notifications look good!

However, the low contrast notifications in the gray 90 and gray 100 themes still aren't correct according to the new spec (let me know if its not possible to achieve this in code then we'll need to evaluate what the options are).

As-is:

To-be:

[asudoh]

@aagonzales Thank you for the review! The problem is that we don't have a theme token that achieves the color switch as you specified. Does this make sense...?

[aagonzales]

Ok so is achieving this style of dark notification not possible? Can it only be the same color as the Gray 10 and white ones?

How can we resolve this? I need some help on the technical side to figure out how we can move forward or what the next round of design parameters should include base off of technical constraints. Can we hard code these values across the themes? Do we need to have a component level tokens (if we can't then why not?). Is the only solution to have one style for all 4 themes?

[asudoh]

@aagonzales Thank you for your further thoughts!

Can it only be the same color as the Gray 10 and white ones?

Yes with the status quo of underlying codebase.

How can we resolve this?

Here are some options I can think of:

1. Implement the notion of component-level theme tokens (The direct limiting factor for implementing your specification, but expecting push-back from some devs as I'm not sure if the team is ready for that notion - CC @emyarod)
2. Create new global theme tokens (Can be achieved only with design spec)
3. Implement code to detect current effective theme and switch style upon that (Expecting push-back from some devs due to its hacky/band-aid nature)

BTW from your below comment, I'm kind of under impression that you may be thinking of changing colors in application code (instead of us doing that) based on the theme of their choice. Please let me know if it's the case.

I think its ok for now if we make these non-themeable.

[aagonzales]

Ok so I think maybe we can go ahead with adding the border style into the components because at least those will be correct in the White and Gray 10 themes. Then in a separate issue/PR we can figure out how to correctly implement the low contrast notifications in the dark themes.

---

Changes needed before merging:

• The toast notifications should not have the border but a drop shadow instead. Only the inline notifications have the border.

• In the Gray 100 and Gray 90 themes the "action" (inline) and "x" (inline and toast) are the wrong colors. Action should be using $link-01 and the x should be the same color as the text.
  • Actually $link-01 doesn't work. Can we have it just use $blue-60?

[asudoh]

Thanks for the updated spec @aagonzales! Fixed.

[aagonzales]

✅ Inline notification is good

❌ For Toast in the gray 90 and Gray 100 themes the close icon x is still not visible. It should be the same color as the text (#161616).

[asudoh]

Oops thank you for catching that @aagonzales! Fixed.

[abbeyhrt]

Looks good to me!

[aagonzales]

Yup, looks good for now. Thanks for fixing!
(Note: we will address the low contrast dark theme issues separately)