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

fix(components): add HCM support to notificiation close icon #10639

Closed
wants to merge 1 commit into from
Closed

fix(components): add HCM support to notificiation close icon #10639

wants to merge 1 commit into from

Conversation

abbeyhrt
Copy link
Contributor

@abbeyhrt abbeyhrt commented Feb 3, 2022

Closes #10586
Closes #10585
REF #10199

Adds HCM for close icon in notification to /components as a part of the styles audit. Was previously added in /styles.

Testing / Reviewing

If you have a Windows computer, using HCM, check that notifications have an accessible close X icon.

@abbeyhrt abbeyhrt requested a review from a team as a code owner February 3, 2022 22:16
@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 56170c9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61fc5459f54f320008493a2f

😎 Browse the preview: https://deploy-preview-10639--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ Deploy Preview for carbon-components-react ready!

🔨 Explore the source changes: 56170c9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61fc5459c96dc700078e6ec5

😎 Browse the preview: https://deploy-preview-10639--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Feb 3, 2022

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 56170c9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61fc54597ba1ea0008fda7d4

😎 Browse the preview: https://deploy-preview-10639--carbon-elements.netlify.app

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having trouble getting a VM up and going, but this looks correct to me

@jnm2377
Copy link
Contributor

jnm2377 commented Feb 7, 2022

Hmmm... I'm reviewing the HCM in white and black since the original issue seemed like it was in white HCM. It looks like the original issue wasn't actually fixed in the white HCM. When I look at the v11 live storybook, the issue is still present. And same for the v10 deploy preview for this PR.

v11 storybook currently

Screen Shot 2022-02-07 at 3 45 25 PM

Screen Shot 2022-02-07 at 3 44 58 PM

PR v10 deploy preview

Screen Shot 2022-02-07 at 3 45 32 PM

Screen Shot 2022-02-07 at 3 44 15 PM

That being said... I'm unsure how to proceed here with since the scope of this PR was to audit the styles and make sure they match, not necessarily fix the bug. Should this be a separate issue? What do you think? @abbeyhrt

@abbeyhrt
Copy link
Contributor Author

abbeyhrt commented Feb 7, 2022

@jnm2377 Good question and great catch! Since these styles don't actually fix the problem, I'll re-open that issue and close this PR. I would rather resolve the issue and then just add whatever the solution is to both places.

@abbeyhrt abbeyhrt closed this Feb 7, 2022
@tw15egan
Copy link
Collaborator

tw15egan commented Feb 7, 2022

@abbeyhrt we may need to edit the high-contrast-mode mixin to target each of the HCM themes. I would have thought ButtonText, a system color, would properly change on the white HCM theme though...

@media screen and (-ms-high-contrast: white-on-black)

https://developer.mozilla.org/en-US/docs/Web/CSS/@media/-ms-high-contrast

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Audit toast-notification styles Audit inline-notification styles
4 participants