-
Notifications
You must be signed in to change notification settings - Fork 55
Conversation
b4f14b1
to
c9e09f8
Compare
c9e09f8
to
72a2e76
Compare
Codecov Report
@@ Coverage Diff @@
## master #1405 +/- ##
==========================================
- Coverage 73.59% 73.23% -0.36%
==========================================
Files 787 794 +7
Lines 5888 5923 +35
Branches 1737 1726 -11
==========================================
+ Hits 4333 4338 +5
- Misses 1549 1579 +30
Partials 6 6
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #1405 +/- ##
==========================================
- Coverage 73.6% 73.49% -0.12%
==========================================
Files 787 793 +6
Lines 5907 5949 +42
Branches 1744 1735 -9
==========================================
+ Hits 4348 4372 +24
- Misses 1553 1571 +18
Partials 6 6
Continue to review full report at Codecov.
|
|
||
export default (siteVars: any): CheckboxVariables => ({ | ||
checkboxBackground: 'transparent', | ||
checkboxBorderColor: siteVars.colors.grey[300], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to use the color scheme here, as this is a fresh new component ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant in teams theme :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we decided not to add the color scheme now and make the component red-lined, let's be sure to track this work
…rdust-ui/react into feat/checkbox # Conflicts: # packages/react/src/themes/teams/components/Icon/iconStyles.ts
|
||
handleClick = (e: React.MouseEvent | React.KeyboardEvent) => { | ||
const { disabled } = this.props | ||
const checked = !this.state.checked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, could be moved in the if statement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will keep as it is for now :)
import { Accessibility } from '../../lib/accessibility/types' | ||
import { checkboxBehavior } from '../../lib/accessibility' | ||
|
||
export interface CheckboxProps extends UIComponentProps, ChildrenComponentProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add conformat test for the new component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, have to add an additional handler for onChange
. Decided to keep it flat to be more obvious
root: { | ||
'aria-checked': props.checked, | ||
role: 'checkbox', | ||
tabIndex: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure, we should not add any attribute if the checkbox is disabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.w3.org/TR/wai-aria-practices/examples/checkbox/checkbox-1/checkbox-1.html
I don't see this in the spec, so decided to skip for the initial implementation
packages/react/src/themes/base/components/Checkbox/checkboxVariables.ts
Outdated
Show resolved
Hide resolved
className={classes.root} | ||
onClick={this.handleClick} | ||
onChange={this.handleChange} | ||
onFocus={this.handleFocus} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this the thing missing for the focus styles? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :) No IDE highlight 😭
Fixes #634.
This PR adds a base implementation of
Checkbox
component with basiccheckboxBehavior
. Also addsstardust-checkmark
icon that is used forCheckbox
component.<Checkbox checked />
<Checkbox checked toogle />
Playground