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
Update unstyled button and list to BEM #2974
Conversation
.usa-button--secondary { | ||
&.usa-button--unstyled { | ||
@include button-unstyled; | ||
} |
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.
Why is it necessary to add the variant button classes before .usa-button--unstyled
in the CSS? It looks like it works without this and keeps the CSS much smaller and shallower.
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.
It was defensive specificity, but I suppose we could remove it
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.
Actually, it does fix some specificity stuff around hover/active/focus
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'm not able to reproduce when I check the button states? Is there something else I should do to see the issue?
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 see one issue with the outline button on hover but can probably be resolved individually by removing the border.
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 want to resolve those issues individually, chain only those classes, or chain all classes (even if most don't need it)?
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'm inclined to leave it as is and apply the specificity across the board, if only to make it clear that the unstyled
variant should always override any other variant styling. But I don't feel super strongly about this.
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.
Made a few tweaks to the mixin in #2976 to make it more robust and fix these two issues. If that looks OK, we can merge it in.
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
That was intentional, but maybe it shouldn't be? |
As for hover issues — I see it in the |
Update button unstyled mixin and remove CSS nesting
Unstyled button preview
Unstyled list preview
.usa-button-unstyled
to.usa-button.usa-button--unstyled
as the unstyled button variant.usa-unstyled-list
to.usa-list.usa-list--unstyled
as the unstyled list variantFooter big
componentFixes #2956