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.

Sign up for GitHub

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

Jump to bottom

fix(button): hcm for ghost icon color contrast #9904

Merged
merged 3 commits into from
Oct 20, 2021
Merged

fix(button): hcm for ghost icon color contrast #9904

merged 3 commits into from
Oct 20, 2021

Conversation

kinueng
Copy link
Contributor

@kinueng kinueng commented Oct 19, 2021 •
edited
Loading

Closes #9903

Add CSS specifically for Windows high contrast mode for the ghost style <Button>

Changelog

Changed

Added CSS specific to the ghost styling for the Button that will apply fill: white to the SVG.

Testing / Reviewing

Before fix

image

After fix

image

@kinueng kinueng requested a review from a team as a code owner October 19, 2021 21:05
@kinueng kinueng requested review from aledavila and abbeyhrt October 19, 2021 21:05
@netlify
Copy link

netlify bot commented Oct 19, 2021 •
edited
Loading

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

🔨 Explore the source changes: e7f7fd6

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

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

@kinueng kinueng force-pushed the high-contrast-mode-button-icon branch from b47e35d to 6642dda Compare October 19, 2021 21:21
@netlify
Copy link

netlify bot commented Oct 19, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 932d9b0

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

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

@netlify
Copy link

netlify bot commented Oct 19, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 932d9b0

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

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

@netlify
Copy link

netlify bot commented Oct 19, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: e7f7fd6

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

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

@netlify
Copy link

netlify bot commented Oct 19, 2021 •
edited
Loading

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: e7f7fd6

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

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

abbeyhrt
abbeyhrt approved these changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@abbeyhrt abbeyhrt left a comment

Choose a reason for hiding this comment

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

Looks great to me! Thank you for the contribution! 🎉

tay1orjones
tay1orjones requested changes Oct 20, 2021
View reviewed changes
Copy link
Member

@tay1orjones tay1orjones left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for submitting this fix! A couple things in addition to the change requested in this review below:

  1. The HCM styles need to be at the very end of the file to ensure they're not overridden
  2. This fix needs to be mirrored in packages/carbon-react -- Edit, sorry I meant packages/styles

packages/components/src/components/button/_button.scss Outdated Show resolved Hide resolved
tay1orjones
tay1orjones approved these changes Oct 20, 2021
View reviewed changes
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.

@kinueng I hope you don't mind - I went ahead and pushed an update to this PR with the suggestions above. Let me know what you think

@kinueng
Copy link
Contributor Author

kinueng commented Oct 20, 2021 •
edited
Loading

@tay1orjones Thank you helping me spruce up my pull request. I don't mind you adding a commit and I get to learn from your changes. I went with the least riskiest change to the stylesheet since I'm so unfamiliar with the Carbon stylesheet codebase.

Retested this PR on my Windows machine using high contrast mode
image

@tay1orjones
Copy link
Member

Fantastic! thanks @kinueng

@kodiakhq kodiakhq bot merged commit 3f7fbdf into carbon-design-system:main Oct 20, 2021
@kinueng kinueng deleted the high-contrast-mode-button-icon branch October 20, 2021 17:24
@kinueng kinueng mentioned this pull request Oct 26, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbeyhrt abbeyhrt abbeyhrt approved these changes

@tay1orjones tay1orjones tay1orjones approved these changes

@aledavila aledavila Awaiting requested review from aledavila aledavila was automatically assigned from carbon-design-system/developers-system

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[a11y]: Button: ghost kind in high contrast mode needs higher contrast ratio
3 participants
@kinueng @tay1orjones @abbeyhrt