-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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): align with 2018 material design spec #12493
Conversation
Caretaker note: aside from the screenshot diffs of checkboxes, this PR will probably cause diff failures for |
src/lib/checkbox/checkbox.ts
Outdated
.monitor(this._inputElement.nativeElement) | ||
.subscribe(focusOrigin => this._onInputFocusChange(focusOrigin)); | ||
this._focusMonitor.monitor(elementRef.nativeElement, true).subscribe(focusOrigin => { | ||
// TODO(paul): support `program`. See https://github.com/angular/material2/issues/9889 |
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.
You should be able to remove this TODO since we no longer control the focus ripple based on the FocusMonitor
.
@@ -1,7 +1,7 @@ | |||
@import './variables'; | |||
|
|||
// The width/height of the checkbox element. | |||
$mat-checkbox-size: $mat-toggle-size !default; | |||
$mat-checkbox-size: 16px !default; |
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.
If the checkbox is getting smaller, I think we need to add an amount of clickable padding around it such that the total size is the same. Two reasons:
- Layouts may shift, which will be very hard to merge
- Our click targets for toggles are already too small as it is (Click targets are too small for some controls #8000)
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 per our discussion on Slack:
- Screenshot diffs will change regardless since the square is smaller. As for layouts changing, it might not be that big of a difference, because we don't have line wrapping on
mat-checkbox
. - Click targets shouldn't be an issue, because checkboxes have a large label next to them that is also clickable.
I think that we should do a presubmit to see how big of an issue this would be.
ea759e3
to
c1bf2a0
Compare
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'll see how the presubmits go before deciding whether to adjust these changes
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
1 similar comment
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
f963f2c
to
2fb4d33
Compare
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
2fb4d33
to
7c92301
Compare
It does, but note how the ruler's "arms" aren't flush against the bottom and top of the control, whereas they are for the radio buttons and slide toggle. I remember triple-checking this, because it seemed like a weird change, but also measuring the mockups themselves and comparing against MDC showed it to be 20px. |
Yeah, I tried out 24px locally and it looked absurd. I sent an email to the material design team asking for a clarification, but I don't expect a response. |
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
7c92301
to
bba5c90
Compare
Hi @crisbeto! This PR has merge conflicts due to recent upstream merges. |
Aligns the checkbox component with the latest Material design spec.
bba5c90
to
c110943
Compare
@crisbeto Did we intentionally remove the persistent ripple on keyboard focus? I see a screendiff complaining about it |
The programmatically-triggered persistent ripple was replaced with a "permanent" ripple that is always inside the ripple container, because we also have to show a ripple on hover. |
I definitely appreciate this change. The box portion of the checkbox was way too big in previous Material versions which made complex forms with lots of controls more awkward. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Aligns the checkbox component with the latest Material design spec.