-
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(menu): Add specificity to selection group class #4172
Conversation
I think this is referencing the wrong bug? Also, should I be able to repro the bug on the new screenshot test page if I un-nest the selector? The page seems to look identical to me in both cases. |
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.
#4170 appears to be unrelated to menu. Did you mean a different issue?
Looks like golden.json
needs to be updated too.
packages/mdc-menu/mdc-menu.scss
Outdated
|
||
.mdc-menu__selection-group-icon { | ||
@include mdc-rtl-reflexive-position(left, 16px); | ||
// Extra specificity required because list-item graphic needed to increase specificity. |
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 don't see .mdc-list-item__graphic
anywhere on the screenshot test page. How is this related?
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: Can you update this comment to something like:
// Extra specificity required to override `display` property on `mdc-list-item__graphic`.
Here's the report to update goldens from: I can now repro the bug by reverting the CSS change on the branch. |
I noticed the checkmark is misaligned in IE but this seems to be a pre-existing issue to this PR, so I logged #4184. |
🤖 Beep boop! Screenshot test report 🚦4 screenshots changed from Details |
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!
packages/mdc-menu/mdc-menu.scss
Outdated
|
||
.mdc-menu__selection-group-icon { | ||
@include mdc-rtl-reflexive-position(left, 16px); | ||
// Extra specificity required because list-item graphic needed to increase specificity. |
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: Can you update this comment to something like:
// Extra specificity required to override `display` property on `mdc-list-item__graphic`.
🤖 Beep boop! Screenshot test report 🚦4 screenshots changed from Details |
…mponents#4172) (cherry picked from commit 1d919ef)
fixes: #4169