Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

feat(chips): Add mixins to customize horizontal padding and icon margins #3530

Merged
merged 10 commits into from
Sep 17, 2018

Conversation

rlfriedman
Copy link
Contributor

No description provided.

Copy link
Contributor

@kfranqueiro kfranqueiro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One doc wording comment, otherwise LGTM.

@@ -208,14 +208,17 @@ Mixin | Description
`mdc-chip-ink-color($color)` | Customizes the text ink color for a chip, and updates the chip's ripple color to match
`mdc-chip-selected-ink-color($color)` | Customizes text ink and ripple color of a chip in the _selected_ state
`mdc-chip-outline($width, $style, $color)` | Customizes the outline properties for a chip
`mdc-chip-outline-width($width)` | Customizes the outline width for a chip
`mdc-chip-outline-width($width, $horizontal-padding)` | Customizes the outline width for a chip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add another sentence:

$horizontal-padding is only required in cases where mdc-chip-horizontal-padding is also included with a custom value.

(This is based on how we documented the outline-width mixin for Button.)

@mdc-web-bot
Copy link
Collaborator

All 389 screenshot tests passed for commit 0fcb7c9 vs. master! 💯🎉

Copy link
Contributor

@lynnmercier lynnmercier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a screenshot test to prevent regressions.

@kfranqueiro
Copy link
Contributor

I would've asked for a screenshot test if it weren't for the fact that we don't have any screenshot tests for chips yet... Do we want to work on that in a separate PR?

@rlfriedman
Copy link
Contributor Author

I'll be adding screenshot tests in a separate PR first, and then will update this to include them for these changes.

@rlfriedman
Copy link
Contributor Author

$right: $mdc-chip-leading-icon-margin-right,
$bottom: $mdc-chip-leading-icon-margin-bottom,
$left: $mdc-chip-leading-icon-margin-left) {
.mdc-chip__icon.mdc-chip__icon--leading {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the .mdc-chip__icon is overly specific in this instance. mdc-chip__icon--leading can only apply to mdc-chip__icon, so you don't need to specify it again.

We did some research offline and realize that sometimes we need to be overly specific about our CSS selectors for icon elements, because we want to override styles applied by material-icons-extended

However,margin is applied by material-icons-extended, so it does not apply in this case.

So remove mdc-chip__icon, pretty please, and re-run the screenshot tests.

$right: $mdc-chip-trailing-icon-margin-right,
$bottom: $mdc-chip-trailing-icon-margin-bottom,
$left: $mdc-chip-trailing-icon-margin-left) {
.mdc-chip__icon.mdc-chip__icon--trailing {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also here.

@@ -77,13 +77,13 @@
}

.mdc-chip__icon--trailing {
margin: 0 -4px 0 4px;
margin: $mdc-chip-trailing-icon-margin-top $mdc-chip-trailing-icon-margin-right $mdc-chip-trailing-icon-margin-bottom $mdc-chip-trailing-icon-margin-left;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is basically duplicating the logic in mdc-chip-trailing-icon-margin...so instead call mdc-chip-trailing-icon-margin from @at-root

and then re-run the screenshots :)

@rlfriedman rlfriedman merged commit 43aeea4 into master Sep 17, 2018
@rlfriedman rlfriedman deleted the feat/chips/icon-margin branch September 17, 2018 20:39
@jamesmfriedman jamesmfriedman mentioned this pull request Sep 26, 2018
49 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants