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: Force PrimaryButton text color in all states #2364

Merged
merged 1 commit into from Oct 20, 2023

Conversation

anicholls
Copy link
Contributor

@anicholls anicholls commented Oct 20, 2023

Summary

The PrimaryButton styling works great when there are no conflicting styles. However, the state pseudo selectors take an unopinionated approach on the text color of the button, which makes it easily overridden by globally scoped css (particularly when using as="a").

In platforms like Docusaurus, it's common to have styling for a:hover. Since PrimaryButton does not have any color set for :hover (or other states), this gets overridden by a simple global specifier, resulting in something that looks like this:

image

While I could add another style for a[type="button"]:hover { color: #fff }, this would cause other problems because we have no way of determining via DOM a PrimaryButton from a SecondaryButton. If this rule were applied to a SecondaryButton, the text would be invisible on a white background.

The simplest approach here IMO was to take a slightly more assertive approach in specifying the color for all states. I don't believe this should have any negative effects, and I don't think it should be a breaking change.

I believe buttons have been largely re-written in v10 👑 , so this may need to be ported to the prerelease branch as well.

Thanks for the consideration!

Release Category

Components


Checklist

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

@anicholls anicholls added the ready for review Code is ready for review label Oct 20, 2023
@cypress
Copy link

cypress bot commented Oct 20, 2023

1 flaky test on run #6345 ↗︎

0 941 2 0 Flakiness 1

Details:

Merge 0175dce into 14663d9...
Project: canvas-kit Commit: 80acdffa17 ℹ️
Status: Passed Duration: 07:35 💡
Started: Oct 20, 2023 8:03 PM Ended: Oct 20, 2023 8:11 PM
Flakiness  cypress/integration/Autocomplete.spec.ts • 1 flaky test

View Output Video

Test Artifacts
... > when a value is entered > when down arrow key is pressed > when the user presses the enter key > when the use hits the "2" key > should change the filtered results Output Screenshots Video

Review all test suite changes for PR #2364 ↗︎

@NicholasBoll
Copy link
Member

v10 is explicit about label color in PrimaryButton, so this fix is only required for v9.

@alanbsmith alanbsmith merged commit c852640 into Workday:master Oct 20, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge ready for review Code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants