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

Create styled radio button #64

Merged
merged 3 commits into from
Oct 16, 2018
Merged

Create styled radio button #64

merged 3 commits into from
Oct 16, 2018

Conversation

kbsletten
Copy link
Contributor

No description provided.

@dustinsoftware
Copy link
Contributor

Version updates need to be done outside the PR

Also, alignment seems off:
image

@JordanSjodinFaithlife
Copy link
Contributor

ezgif-5-6ab104902028

Should this be flashing white like this? Safari on Mac 10.13

@kbsletten
Copy link
Contributor Author

@JordanSjodinFaithlife That does seem to also happen on Checkbox (which I copied), I'll see if I can't figure out what's causing it.

@@ -22,6 +22,10 @@ export const CheckboxContainer = styled.button`
min-height: 16px;
background: transparent;

&:active {
color: buttontext;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my solution to the "white flash" that Safari has for these. By default Safari applies "activebuttontext" color to buttons with the ":active" pseudo-class. I'm not convinced this is the best idea, however because we're now enforcing a "buttontext" color on these controls. This makes one additional step to coloring the text of these elements, though it's arguably more discoverable than the Safari thing because it will act the same in all browsers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dustinsoftware is there a more "styled-ui" way of doing this, in your opinion?

Copy link
Contributor

@dustinsoftware dustinsoftware left a comment

Choose a reason for hiding this comment

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

If we need to we can revisit the text color. I’m fine with it as is.

@dustinsoftware dustinsoftware merged commit 805ce6a into Faithlife:master Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants