-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(switch): Merge updated switch into master #3214
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3214 +/- ##
==========================================
+ Coverage 98.12% 98.13% +0.01%
==========================================
Files 101 104 +3
Lines 4421 4507 +86
Branches 585 587 +2
==========================================
+ Hits 4338 4423 +85
- Misses 83 84 +1
Continue to review full report at Codecov.
|
chore(switch): Merge master into feat/switch-update
Screenshot test report
|
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 started looking this over but will need to continue later. Pointed out a few things I noticed.
.travis.yml
Outdated
@@ -6,6 +6,7 @@ branches: | |||
# This prevents excessive resource usage and CI slowness. | |||
only: | |||
- master | |||
- feat/switch-update |
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.
This should be removed in the merge
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 do.
@@ -26,6 +26,7 @@ const checkbox = new mdc.checkbox.MDCCheckbox(document.querySelector('.mdc-check | |||
import { checkbox } from 'material-components-web'; | |||
const checkbox = new checkbox.MDCCheckbox(document.querySelector('.mdc-checkbox')); | |||
``` | |||
> NOTE: Since switch is a reserved word in JS, switch is instead named `switchControl`. |
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.
Add a newline before this line
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 do.
<div class="mdc-switch__thumb-underlay"> | ||
<div class="mdc-switch__thumb"> | ||
<input type="checkbox" id="basic-switch" class="mdc-switch__native-control" role="switch"> | ||
</div> |
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.
The basic usage section seems to be missing the Styles (for imports) and JavaScript Instantiation sections
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.
Good catch, will add!
packages/mdc-switch/README.md
Outdated
`mdc-switch__thumb` | Mandatory, for the thumb element. | ||
`mdc-switch__native-control` | Mandatory, for the hidden input checkbox. | ||
|
||
## Style Customization |
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.
There's already a style customization heading above, I think this one shouldn't be here
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.
Thanks, will remove.
packages/mdc-switch/README.md
Outdated
|
||
## Usage within Web Frameworks | ||
|
||
If you are using a JavaScript framework, such as React or Angular, you can create a switch for your framework. Depending on your needs, you can use the _Simple Approach: Wrapping MDC Web Vanilla Components_, or the _Advanced Approach: Using Foundations and Adapters_. Please follow the instructions [here](../../docs/integrating-into-frameworks.md). |
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.
Capitalize "Switch" in "create a switch for your framework"
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 do.
| `setDisabled(disabled: boolean) => void` | Sets the disabled value of the native control and updates styling to reflect the disabled state. | | ||
| `handleChange() => void` | Handles a change event from the native control. | | ||
|
||
### `MDCSwitchFoundation` Event Handlers |
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.
Shouldn't this be documented under the component rather than the foundation?
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 just re-read this and realized why it's here; I had misread it earlier. This is basically an example of the sort of thing Pat was saying we should add, so this can stay here.
Screenshot test report
|
@@ -8,17 +8,22 @@ | |||
"material design", | |||
"switch" | |||
], | |||
"main": "index.js", |
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.
please point this to dist/mdc.switch.js
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.
Done
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 can't find a single other package we're doing this for in MDC Web and thus think this is a mistake...
Can you explain this @moog16 ? At the very least I don't think we should be making one package inconsistent from the rest right now.
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.
Okay, I just found #3245, which explains this.
I don't think we should be rushing to do that just yet - please revert this here.
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.
sorry @rlfriedman! I jumped the gun :)
Screenshot test report
|
This reverts commit 9becfc8.
Screenshot test report
|
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.
This will need a proper BREAKING CHANGE description in the commit description when merging, e.g.:
BREAKING CHANGE: MDC Switch DOM structure and Sass APIs have changed, and JavaScript APIs have been added. See the documentation for more information.
Screenshot test report
|
BREAKING CHANGE: MDC Switch DOM structure and Sass APIs have changed, and JavaScript APIs have been added. See the documentation for more information.
Fixes #2825