-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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(input): single-line hints overflowing the parent #4107
Conversation
Fixes the input container hints causing an overflow on the parent by a couple of pixels. Note that this can be fixed alternatively by setting `line-height: 1` on the wrapper, however it causes multi-line hints to be a little too compact. I went with 1.2 since it seems to be the closest to the browser defaults for `normal`. Fixes angular#4051.
position: absolute; | ||
font-size: 75%; | ||
top: 100%; | ||
width: 100%; | ||
margin-top: -$mat-input-wrapper-spacing; | ||
margin-top: -$line-height; | ||
line-height: $line-height; |
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 not change the line-height
on .mat-input-container
from normal
to 1.2em
instead of overriding here?
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 wanted to avoid breaking other parts of the input container since these changes are only relevant to the hints.
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 think it should be ok, most browsers have normal = 1.2. Using 1.2 explicitly will make sure its consistent in all browsers
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.
Since changing the one on the container causes other elements to overflow I'm fine with doing this
src/lib/input/input-container.scss
Outdated
@@ -43,7 +42,7 @@ $mat-input-underline-disabled-background-image: | |||
// Global wrapper. We need to apply margin to the element for spacing, but | |||
// cannot apply it to the host element directly. | |||
.mat-input-wrapper { | |||
margin: $mat-input-wrapper-spacing 0; | |||
margin: 1em 0; |
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 did you remove the constant?
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.
After the changes I did below, it was only being used in one place which makes it pointless.
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 think we often prefer constants even if they're only used once since they describe what the value is supposed to represent.
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes the input container hints causing an overflow on the parent by a couple of pixels. Note that this can be fixed alternatively by setting
line-height: 1
on the wrapper, however it causes multi-line hints to be a little too compact. I went with 1.2 since it seems to be the closest to the browser defaults fornormal
.Fixes #4051.