-
Notifications
You must be signed in to change notification settings - Fork 6.8k
bug(focus indicators): Focus indicators are now positioned. #19345
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
Conversation
crisbeto
left a comment
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 @jelbourn might have an opinion on this.
|
@jelbourn Before merging this, let's close on whether the style |
jelbourn
left a comment
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.
Replied via email, but I'm good with adding the relative style in mat-core (assuming the TGP works out)
|
|
||
| // The focus indicator should match the bounds of the entire button. | ||
| .mat-mdc-focus-indicator { | ||
| @include mat-fill(); |
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.
Before this change, this element had essentially no styles, and thus the focus indicator actually rendered relative to the parent button. Now that we've added position: relative to this element, the fact that it has no styles is problematic (the focus indicator doesn't render with the correct bounding box). Consequently, use mat-fill to make this element match the bounds of the entire button.
|
TGP LGTM, sent the link to you @jelbourn. |
jelbourn
left a comment
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, just minor naming comment
quick fix date picker focus indicator bug.
|
Addressed naming comment, rebased and squashed commits. |
jelbourn
left a comment
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
…lesheets are ordered improperly in Karma unit tests.
|
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. |
.mat-rippleis positioned in order to create an easy API for developers, every.mat-focus-indicatorshould be positioned. This way, when developers add the class.mat-focus-indicatorto their own custom focusable elements they do not also need to position the element..mat-focus-indicator, a few components that setposition: absoluteon the associated element need to have their specificity bumped.