-
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
fix(switch): Remove checkbox references from SCSS #352
fix(switch): Remove checkbox references from SCSS #352
Conversation
@@ -725,6 +725,10 @@ Concretely: | |||
contains javascript, ensure that its component namespace is exported within | |||
`material-components-web`, and it is registered with `mdc-auto-init`. Lastly, remember to add it | |||
to `package.json` of `material-components-web`. | |||
- Ensure that the correct **commit subject** for the package is added to the |
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.
Do you mean if this package is added to config.validate-commit-msg.scope.allowed
then its commit message will be checked and if I wrote wrong commit message like "feat(mdc-grid-list): Implement mdc-grid-list (#47)" it won't go through?
What about cases if I forget to add this package to that config file, will that commit message go through? As far as what I am concerning, this should be an 'opt-out' instead 'opt-in' check.
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.
Do you mean if this package is added to config.validate-commit-msg.scope.allowed then its commit message will be checked and if I wrote wrong commit message like "feat(mdc-grid-list): Implement mdc-grid-list (#47)" it won't go through?
That's correct!
What about cases if I forget to add this package to that config file, will that commit message go through?
It won't. So this ensures that either you:
- Use a pre-defined scope
- Add the new component scope to the config
It should be a general comment not a request review
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. We may want to split out infra work into separate commits in the future though, for the sake of commit history sanity. No need to do it for this one, though, it would be a hassle to split it out now.
Approved! |
SGTM I included it in here since it doesn't really break anything. Also I'm going to update the list to include |
- Remove all references to mdc-checkbox in mdc-theme-dark mixin - Add `mdc-switch-transition` function and replace calls to `mdc-checkbox-transition` with that function - (techdebt) Update cross-env and cz-conventional-changelog - (techdebt) Add allowed scopes to validate-commit-msg config, and docs on how to add new scopes when a component is added. Fixes #322
362b47f
to
9f08315
Compare
mdc-switch-transition
function and replace calls tomdc-checkbox-transition
with that functionon how to add new scopes when a component is added.
Fixes #322