material-components / material-components-web

Modular and customizable Material Design UI components for the web
https://material.io/develop/web
MIT License
17.11k stars 2.15k forks source link

[TextField + Icon] Leading icon has incorrect padding #6618

Open youra-h opened 3 years ago

youra-h commented 3 years ago

Bug report

Steps to reproduce

Taken from discord image For icon button necessary to use mdc-text-field__icon--trailing.margin-right: 4px; Stille like this for prefix and suffix

/* TEXTFIELD PREFIX SUFFIX */
.mdc-text-field__affix--prefix {
    padding-right: 4px;
}
.mdc-text-field__affix--suffix {
    padding-left: 4px;
}

And image

image

Final code

/* TEXTFIELD HELPER LINE */
.mdc-text-field+.mdc-text-field-helper-line {
    justify-content: left;
    padding: 0 14px;
}

/* TEXTFIELD ICONS */

.mdc-text-field {
    .mdc-icon-button {
        @include icon-button.size(44px);
    }
}

.mdc-text-field__icon--leading,
.mdc-text-field__icon--trailing {
    padding: 10px;
}

.mdc-text-field__icon--leading {
    margin: 0 4px;
}

.mdc-text-field__icon--trailing {
    margin: 0 4px;
}

.mdc-text-field--with-leading-icon {
    &.mdc-text-field--filled {
        .mdc-floating-label {
            left: 52px;
        }
    }
}

/* TEXTFIELD PREFIX SUFFIX */
.mdc-text-field__affix--prefix {
    padding-right: 4px;
}
.mdc-text-field__affix--suffix {
    padding-left: 4px;
}

Actual behavior

Expected behavior

Screenshots

Your Environment:

Software Version(s)
MDC Web 8.0.0
Browser Opera/Chrome
Operating System Windows

Additional context

Possible solution

youra-h commented 3 years ago
/* TEXTFIELD */
.mdc-text-field-helper-line,
.mdc-text-field {
    --padding-text-field: 14px;
}

.mdc-text-field {
    padding: 0 var(--padding-text-field, 16px);
}

/* TEXTFIELD LABEL */
.mdc-text-field--filled {
    .mdc-floating-label {
        left: var(--padding-text-field, 16px);
    }
}

/* TEXTFIELD HELPER LINE */
.mdc-text-field+.mdc-text-field-helper-line {
    justify-content: left;
    padding: 0 var(--padding-text-field, 16px);
}

/* TEXTFIELD ICONS */

.mdc-text-field--with-trailing-icon {
    padding-left: var(--padding-text-field, 16px);
    padding-right: 0;
}

.mdc-text-field--with-leading-icon {
    padding-right: var(--padding-text-field, 16px);
    padding-left: 0;
    &.mdc-text-field--filled {
        // GOTO here you have to calc
        .mdc-floating-label {
            left: 50px;
        }
    }
}

// GOTO here you have to calc
.mdc-text-field {
    .mdc-icon-button {
        @include icon-button.size(44px);
    }
}

.mdc-text-field__icon--leading,
.mdc-text-field__icon--trailing {
    padding: 10px;
}

.mdc-text-field__icon--leading {
    margin: 0 2px 0 4px;
}

.mdc-text-field__icon--trailing {
    margin: 0 4px 0 2px;
}

/* TEXTFIELD PREFIX SUFFIX */
.mdc-text-field__affix--prefix {
    padding-right: 4px;
}
.mdc-text-field__affix--suffix {
    padding-left: 4px;
}
asyncLiz commented 3 years ago

@sayris could you take a look into this if you have time? I believe you fixed the trailing icon touch target and I think the leading icon needs fixed as well.

sayris commented 3 years ago

Hi @devslyfox -- thanks for flagging this issue!

Can you confirm that this bug is about the discrepancies in padding between the leading and trailing icon in the textfield (and so a fix would increase the padding on the leading icon to match that of trailing icon)? Unfortunately, I can't discern what is adjusted in the screenshots following the "I think this is how it should be" message, so want to make sure I understand the issue fully before working on a fix based on my assumptions :)

Thanks!

youra-h commented 3 years ago

Look, this post is not related to the current question. This is a discord picture from some user. But it also shows that the icon goes beyond the boundaries. image

I did like this: Leading icon image image Leading button image

Trailing icon image image Trailing button image

asyncLiz commented 3 years ago

@sayris here's a jsbin showcasing the touch target issue: https://jsbin.com/posiyaqamu/edit?html,output

The leading icon does not have a 48x48 touch target while the trailing icon does, due to it using margin instead of padding.

Screen Shot 2020-11-17 at 11 06 12 AM Screen Shot 2020-11-17 at 11 06 15 AM

@devslyfox the icon's touch target should meet the edge of the text field, there should not be any additional margin added. If a 48x48 ripple is added, that will result in the ripple meeting the edge of the text field container as well. You can reduce the size of the ripple if this is undesirable, or add your own margin if you wish the ripple to remain the same size.

At the moment we don't have explicit guidance from design on what the icon's ripple should look like for text field.

wouterSkepp commented 3 years ago

The leading icon does not have a 48x48 touch target while the trailing icon does, due to it using margin instead of padding.

I'd like to chime in that the icon--leading and icon--trailing do not have a 48x48px touch target - It is a side-effect of using material-icon. If one would swap out the iconset for font-awesome (for example), it is not enforced, offsetting the floating label.

I would suggest adding the sizing to the relevant .mdc-text-field__icon This floating-label offset is also happening in the select component with .mdc-select__icon.