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

feat(checkbox): Updates to experimental checkbox #1135

Merged
merged 10 commits into from
Sep 28, 2018
Merged

feat(checkbox): Updates to experimental checkbox #1135

merged 10 commits into from
Sep 28, 2018

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Sep 21, 2018

Closes #1118

http://carbon-dev-environment-optimistic-kangaroo.stage1.mybluemix.net/demo/checkbox

Hey @jeanservaas this should be pretty close to the sketch file(without having values like .8723864), but let me know if you see anything that needs tweaks. Specifically wasn't sure on the "checked focus" state. The blue border looks pretty thin.

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 once @jeanservaas signs this off. Thanks @alisonjoseph!

@tw15egan
Copy link
Collaborator

tw15egan commented Sep 24, 2018

Focus states are pretty hard to determine when using the keyboard to navigate. This does not seem likely to pass a11y.

https://www.w3.org/WAI/GL/low-vision-a11y-tf/wiki/Contrast_(Minimum)#Focus_Indicators

It either needs to be greater than 3px, or have a 4.5 contrast ratio.

screen shot 2018-09-24 at 12 31 55 pm

Also, seems like intermediate state does not have a focus state.

@alisonjoseph
Copy link
Member Author

Good catch @tw15egan, let me know how you want to adjust the focus state @jeanservaas

@alisonjoseph
Copy link
Member Author

@jeanservaas This is probably why the original duo-component checkbox had the following focus state.
screen shot 2018-09-24 at 1 28 34 pm

@jeanservaas
Copy link
Collaborator

Hey Alison,

  • The type next to the enabled check box should be Gray 100 (hex#171717) right now it's hex #152935.

  • Can you move the type down 1px (it looks like it's riding high on the vertical center now)

  • You're right, the selected focus state is not legible. There should be a 3px boarder around the checked/focus state -- if there is and it's not enough, try adding another pixel.

  • The check in the check box looks like it got vertically stretched by about a pixel. Just seems like it's rendering a little fuzzy. Don't have a solution here, so we don't have to get hung up on it if there's no fix.

Last disabled check box's stroke under (Checkbox (input >label) is the Gray 100, it should be the disabled color (Gray 30 / #BEBEBE)

screen shot 2018-09-24 at 1 33 34 pm

@jeanservaas
Copy link
Collaborator

@alisonjoseph Oh! I just saw that for the selected focus state, I like that, better than the stroke around it! I'm trying to think of a UX reason why we wouldn't be able to have that... maybe because a blue 60 fill is associated with a links/buttons. I'll ask around. We'd also have to change the radio button if we changed this.

@alisonjoseph
Copy link
Member Author

alisonjoseph commented Sep 24, 2018

screen shot 2018-09-24 at 2 10 25 pm

screen shot 2018-09-24 at 2 10 16 pm

updated with the correct 3px border. but can always switch to the blue background. trying to push the staging link now 🤞

@jeanservaas
Copy link
Collaborator

@alisonjoseph

Almost there... few small things

  • wondering if that text color the gray 100 (171717) -- i.e. the "checkbox" and the "indeterminate checkbox," still looks a little lighter and a little bluish in the grab

  • also, the corner radius... now that the box is slightly bigger than the original artwork in sketch, the corner is a bit pointier than the original. Again, the checkbox isn't live in sketch so I can't check the corner radius... but it looks like it's about a 2 in sketch. Maybe we could try to match this

  • just make sure dash horizontal dash on indeterminate checkbox is vertically centered. In the original link it rides about a pixel high.

thanks for your patience!!!

@alisonjoseph
Copy link
Member Author

Double checked the text color, just need confirmation on 1px or 2px for the border-radius. It almost seems like its 1.5 in sketch.

here is a side by side of 1px vs 2px
@jeanservaas
screen shot 2018-09-27 at 4 25 13 pm

Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

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

looks good!

@alisonjoseph alisonjoseph merged commit 7fd0895 into carbon-design-system:master Sep 28, 2018
@alisonjoseph alisonjoseph deleted the checkbox-x branch September 28, 2018 15:36
@carbon-bot
Copy link
Contributor

🎉 This PR is included in version 9.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Experimental Component: Checkbox
6 participants