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

Checkbox #257

Merged
merged 34 commits into from Dec 16, 2018

Conversation

Projects
None yet
4 participants
@macor161
Copy link
Contributor

macor161 commented Nov 27, 2018

Fixes #50

image

Let me know if something needs to be changed.

@bpierre bpierre self-requested a review Nov 27, 2018

bpierre added some commits Dec 5, 2018

Springs tweaks
- Remove the precision
- Make `swift` faster
- Add `instant`, which is even faster than `swift`. Useful for super
  small elements like a checkbox tick.
Checkbox tweaks
- Follow the exact design made for the checkbox (from aragon/design).
- Use react-spring for consistent animation with the other components.
- Use a component for the tick mark instead of CSS, so that IconCheck
  can be used.
- Add a focus ring.

Using a component + a custom spring makes it impossible to only use CSS,
that’s why the <input type="checkbox> has been replaced by a button and
ARIA attributes.

It follows the required behavior for the ARIA role [1]:

- A click event toggles its value.
- Using the space bar or the enter key while the item is focused
  will toggle its value.

Missing states: disabled, mixed.

[1] https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/checkbox_role
@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Dec 6, 2018

Hi @macor161, thanks for contributing!

I added a few commits to tweak its visual design and some other things. Let me know if that works for you!

Changes:

  • Follow the design from the aragon/design Sketch file.
  • Use react-spring for consistent animation.
  • Use a <IconCheck /> for the tick mark instead of CSS.
  • Add a focus state.

Using a component + a custom spring makes it impossible to only use CSS, that’s why the <input type=checkbox> has been replaced by a button and ARIA attributes.

It follows the required behavior for the checkbox role:

  • A click event changes its state.
  • Using the space bar or the enter key while the item is focused changes its state.

None of these events needed to be actually implemented, because a <button type=button> is used and already does it.

Missing states: disabled, mixed ([−]). /ping @jounih

2018-12-06 18 49 33

@bpierre bpierre requested review from sohkai , jounih and AquiGorka Dec 6, 2018

@bpierre

bpierre approved these changes Dec 6, 2018

bpierre added some commits Dec 6, 2018

@sohkai

sohkai approved these changes Dec 8, 2018

Copy link
Member

sohkai left a comment

Looks great!

Added it to the gallery :).

&:focus:not(:focus-visible) ${FocusRing} {
display: none;
}
&:focus-visible ${FocusRing} {

This comment has been minimized.

@sohkai

sohkai Dec 8, 2018

Member

Nice!

import theme from '../../theme'
import { springs } from '../../utils'

class CheckBox extends React.Component {

This comment has been minimized.

@sohkai

sohkai Dec 8, 2018

Member

One small nitpick: I think Checkbox (lowercase b) might be better, since it's more commonly used.

This comment has been minimized.

@bpierre

bpierre Dec 8, 2018

Member

Ah, I made the change because both Android and macOS are using “CheckBox”, and that’s also what we were using in the issue #50.

I don’t really have any opinion on this, but I want to go for the most obvious one for users. What do you think?

This comment has been minimized.

@AquiGorka

AquiGorka Dec 11, 2018

Contributor

I vote for lowercase b as @sohkai suggested (ref)

This comment has been minimized.

@sohkai

sohkai Dec 11, 2018

Member

Let's use Checkbox then :). It seems to be the canonical way to spell it.

This comment has been minimized.

@bpierre

bpierre Dec 11, 2018

Member

I found at least one toolkit using Checkbox (Flutter), let’s go for it then.

<label><CheckBox onChange={onChange} /> Third</label>
</div>
)
```

This comment has been minimized.

@sohkai

sohkai Dec 8, 2018

Member

Could we add descriptions for the proptypes here? In particular, I'm not sure what "mixed" means.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Dec 8, 2018

Ahh, just noticed, on chrome, it seems to enable the focus ring when activated by the mouse as well :(.

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Dec 8, 2018

@sohkai When clicking on a checkbox? It should only appear if the window is focused out, or when pressing a key.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Dec 10, 2018

@sohkai When clicking on a checkbox? It should only appear if the window is focused out, or when pressing a key.

@bpierre Yes... either on the checkbox itself or on its label:

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Dec 10, 2018

@sohkai oh got it, it’s because I have experimental web features enabled in Chrome… I thought :focus-visible was supported but it’s not the case yet. I’ll replace it with another solution.

swift: { mass: 0.5, tension: 800, friction: 30 },

// Super fast speed spring, for interactions that feel instant (e.g. a
// chekbox tick).

This comment has been minimized.

@AquiGorka

AquiGorka Dec 11, 2018

Contributor

Typo here: chekbox

@AquiGorka
Copy link
Contributor

AquiGorka left a comment

Would it make sense to include in the description that we use the React way of controlled components and that's why there is no need for name/value?

@bpierre

This comment has been minimized.

Copy link
Member

bpierre commented Dec 11, 2018

Would it make sense to include in the description that we use the React way of controlled components and that's why there is no need for name/value?

Yes totally agree, we should talk about the general principles we follow when designing the toolkit API 👍

bpierre and others added some commits Dec 11, 2018

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Dec 14, 2018

@bpierre Oops, forgot to push the changes for the gallery page earlier :). Also fixed a linting warning with using theme as the default export.

@bpierre bpierre merged commit f41502c into aragon:master Dec 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
<label>
<Checkbox
checked={first && second}
indeterminate={Boolean(first ^ second)}

This comment has been minimized.

@bpierre
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment