-
Notifications
You must be signed in to change notification settings - Fork 42
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): Add checkbox demo #46
Conversation
return ( | ||
<div> | ||
<div className='mdc-checkbox demo-checkbox' ref={this.initCheckbox}> | ||
<input type='checkbox' |
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 think I would make one of these checkboxes checked, so our hero image is nice
src/CheckboxPage.js
Outdated
render() { | ||
return ( | ||
<div> | ||
{this.renderCheckboxVariant('Checked', {checked: true})} |
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: you could drop this "checked" variant if you change the hero image
src/CheckboxPage.js
Outdated
return ( | ||
<div> | ||
{this.renderCheckboxVariant('Checked', {checked: true})} | ||
{this.renderCheckboxVariant('Indeterminate', {indeterminate: true})} |
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: intedeterminate is more a state than a variant.....but whatevs
(theres really only 1 variant of checkbox, which makes this page a little sparse)
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 think it's worth showing indeterminate, otherwise people might not even realize we have support for it.
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.
Keeping indeterminate
src/images/buttons_180px.svg
Outdated
@@ -0,0 +1,57 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
FYI the update to the button front-page image and the relevant CSS is already in master now.
src/App.js
Outdated
@@ -51,6 +54,7 @@ class App extends Component { | |||
<HeaderBar isTopPage /> | |||
<ul id='catalog-image-list' className='mdc-image-list standard-image-list mdc-top-app-bar--fixed-adjust'> | |||
{this.renderListItem('Button', buttonImg, `${PUBLIC_URL}/button`)} | |||
{this.renderListItem('Checkbox', checkboxImg, `${PUBLIC_URL}/checkbox`)} |
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.
FYI you can reference the image URL directly here and don't need PUBLIC_URL anymore, and don't need the image import above. (You'll probably see when you merge with master)
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.
LGTM, but you have a double import in App.js
Add checkbox demo. Update button catalog icon to use large icon. Remove catalog icon size constraints.