feat(text-field): Add support for leading/trailing icons at the same time #3451
Conversation
|
🤖 Beep boop! Screenshot test report 🚦52 screenshots changed from Details52 Added:
|
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
williamernest
force-pushed
the
fix/text-field/both-icons
branch
from
a536d33 to
8b80f1c
Compare
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
|
Once this is merged will it also pave the way for leading icons on select fields? The spec says nothing about not supporting icons on select fields as far as I can see... |
|
@GusRuss89 Yes. The goal is to add support for leading icons and invalid styles to the current native select. We're also adding an enhanced select with the menu component that should also have feature parity with the text field. |
|
@williamernest That would be great to have also! Native selects are cool, but with not much control |
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
packages/mdc-textfield/_mixins.scss
Outdated
| @@ -382,6 +382,14 @@ | |||
| @include mdc-text-field-icon-horizontal-position_(right, $mdc-text-field-dense-icon-position, $mdc-text-field-dense-icon-padding); | |||
| } | |||
|
|
|||
| @mixin mdc-text-field-with-both-icons_ { | |||
| @include mdc-text-field-icon-horizontal-position-two-icons_($mdc-text-field-icon-position, $mdc-text-field-icon-padding, $mdc-text-field-trailing-icon-position,$mdc-text-field-icon-padding); | |||
There was a problem hiding this comment.
Missing space after last comma
| @@ -42,11 +42,11 @@ const icon = new MDCTextFieldIcon(document.querySelector('.mdc-text-field-icon') | |||
| ``` | |||
| ## Variants | |||
|
|
|||
| Leading and trailing icons can be applied to text fields styled as `mdc-text-field--box` or `mdc-text-field--outlined`. To add an icon, add the relevant class (either `mdc-text-field--with-leading-icon` or `mdc-text-field--with-trailing-icon`) to the root element, add an `i` element with your preferred icon, and give it a class of `mdc-text-field__icon`. | |||
| Leading and trailing icons can be applied to text fields styled as the default or `mdc-text-field--outlined`. To add an icon, add the relevant class (either `mdc-text-field--with-leading-icon` and/or `mdc-text-field--with-trailing-icon`) to the root element, add an `i` element with your preferred icon, and give it a class of `mdc-text-field__icon`. If using 2 icons, the first icon will be positioned before the input text, and the second icon will be positioned after. | |||
There was a problem hiding this comment.
Remove "either" from the parenthetical
Also... in reading the last sentence... is this supposed to be a "will", or a "must"? Like, is this talking about how the markup is supposed to be arranged?
| .mdc-text-field__icon { | ||
| @include mdc-theme-prop(color, $color); | ||
| &:first-of-type { |
There was a problem hiding this comment.
Just pointing out this will only work as long as it's safe to assume that the icons always use a distinct type (e.g. i, svg, etc., not div), since first-of-type and last-of-type apply against the element type, not the selector. I think this is a safe assumption...
There was a problem hiding this comment.
Maybe add a class like mdc-text-field__icon--leading and mdc-text-field__icon--trailing can prevent that?
There was a problem hiding this comment.
I can work around the last-of-type issue using a sibling selector
.mdc-text-field__icon ~ .mdc-text-field__icon {
color: red;
}
| @@ -22,9 +22,9 @@ | |||
|
|
|||
| // Public mixins | |||
|
|
|||
| @mixin mdc-text-field-icon-color($color) { | |||
| @mixin mdc-text-field-icon-color($color, $onlyTrailing: false) { | |||
There was a problem hiding this comment.
Is there a reason why we're singling out trailing like this?
Would it make more sense to have there be 2 color parameters? i.e. $color, $trailingColor: $color where if you specify one it applies to both, but if you specify two they apply to leading/trailing?
(Could also do $trailingColor: null and still conditionalize the output like you're currently doing.)
There was a problem hiding this comment.
This was only used to style the trailing icon when in an invalid state, since that's the only time the 2 icons have different colors. I don't think it makes sense to support passing in 2 colors, since majority of the time the icons will always be the same color. There's only 1 time we need an exception (and to be honest, I'm not sure why leading icons don't turn red when the text field is invalid).
|
This looks OK from a styling perspective, but I think there are things on the JS side that this isn't taking into account... The leading/trailing icon also has a controller/foundation, and text field is currently still assuming that there's only ever one of them to manage. This affects a few things:
|
Codecov Report
@@ Coverage Diff @@
## master #3451 +/- ##
==========================================
- Coverage 98.41% 98.39% -0.02%
==========================================
Files 120 120
Lines 5036 5058 +22
Branches 620 627 +7
==========================================
+ Hits 4956 4977 +21
- Misses 80 81 +1
Continue to review full report at Codecov.
|
kfranqueiro
changed the title
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
|
🤖 Beep boop! Screenshot test report 🚦20 screenshots changed from Details20 Added:
|
fixes: #3302