Skip to content
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): Fix floating label width #689

Merged
merged 7 commits into from
Jun 21, 2016
Merged

fix(input): Fix floating label width #689

merged 7 commits into from
Jun 21, 2016

Conversation

jean-merelis
Copy link
Contributor

@jean-merelis jean-merelis commented Jun 15, 2016

Fix floating label to resize the width to use all available space.

Fixes #688

Fix floating label to resize the width to use all available space
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 15, 2016
@crisbeto
Copy link
Member

You should keep in mind that this will cause an overflow on IE and Edge. They don't consider the scale when determining whether to overflow the parent.

@jean-merelis jean-merelis changed the title fix(input): Fix floating label width fix(input): Fix floating label width (#688) Jun 15, 2016
@jean-merelis jean-merelis changed the title fix(input): Fix floating label width (#688) fix(input): Fix floating label width Jun 15, 2016
@@ -34,7 +34,8 @@ $md-input-underline-disabled-background-image: linear-gradient(to right,
visibility: visible;
padding-bottom: 5px;
transform: translateY(-100%) scale(0.75);

width: 133%; // 100/75 of the original space now available
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of magic number, please use a scale constant and make the width 100% / placeholderScaleFactor.

Also please transition the width as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not a magic number. It is just 100% / placeholderScaleFactor. 100/75=1.33

Copy link
Contributor

Choose a reason for hiding this comment

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

What I'm saying is that this is a good time to move the 0.75 out into a variable and use that variable in the width calculation. This will clean up stuff and make it clear what the intent is. The comment 100 / 75 doesn't tell us anything about where 75 came from. Nor what is the purpose of 75.

Also, if we change the 0.75 scale, we can't possibly know what else to change.

@@ -34,7 +34,9 @@ $md-input-underline-disabled-background-image: linear-gradient(to right,
visibility: visible;
padding-bottom: 5px;
transform: translateY(-100%) scale(0.75);

transition: width 150ms ease-in-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

So this will overwrite or be overwritten by the transition already on the .md-input-placeholder. The transition should be added to line 125 AND use material transition functions.

@hansl
Copy link
Contributor

hansl commented Jun 15, 2016

@crisbeto: I think in those case we'll see the same behaviour as before this change, which is fine by me (for now).

@crisbeto
Copy link
Member

We had a similar issue on Material 1, even though the label is scaled down to 75%, IE/Edge still consider it as if it was 100%. Then when you set it to 133% it overflows. See angular/material#8116

@jelbourn
Copy link
Member

@hansl are you able to see if this is an issue in IE / Edge?

@@ -20,6 +20,9 @@ $md-toggle-padding: 8px !default;
// Width and height of input toggles
$md-toggle-size: 20px !default;


$md-input-floating-placeholder-scale-factor: 0.75 !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Variables that are specific to a single component should live in that component's SCSS.

Just move this to the file where it's used.

@hansl
Copy link
Contributor

hansl commented Jun 20, 2016

Re IE/Edge I haven't had a chance to get my Windows Machine up last week. Will check it out tomorrow on a local VM.

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

I'm installing a VM to test this on IE 11. Will come back when done.

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

This seems to be working fine on IE11. Syncing up with Robert for more info.

Proof:

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

@crisbeto: ^ seems to work correctly. If you have something to add before we merge this in :)

Please note our DOM is largely different than Material 1, and much simpler/less reliant on hacks due to Shadow DOM and compartmentalization.

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

As soon as we clarify the situation with IE11/Edge I'll merge this in. :)

@crisbeto
Copy link
Member

I'll try cloning it to check as well.

@crisbeto
Copy link
Member

Alright, it works as it is now, but it starts overflowing if the any of the parents get overflow: auto. I'm not if it's an issue in this case, but here's what it looks like:

a

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

Interesting. By putting overflow: hidden on the md-input itself we mitigate setting it to anything above, which would be okay I guess. The worst case would be if a customer sets overflow: auto to the md-input itself, which we can say "don't do that" since there's no real reason to do it. And it would be fine if he sets it on the element above (and beyond). @crisbeto: What do you think?

@jean-merelis: Can you add overflow: hidden to the :host?

@crisbeto
Copy link
Member

Yeah I'd say that setting a default overflow would be fine, since it's up to the end user to determine whether to override it in that case.

@hansl
Copy link
Contributor

hansl commented Jun 21, 2016

Thanks Kristiyan. Sorry if this was such a mess, but better be right the first time :)

After discussion with @robertmesserle seems like this is the right solution. I'll merge it as soon as Travis finishes.

@hansl hansl added the pr: lgtm label Jun 21, 2016
@hansl hansl merged commit cf2703c into angular:master Jun 21, 2016
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input floating label should expand when entering floating state
5 participants