Skip to content

Animate size of label text over text fields. - #7241

Merged
mpcomplete merged 5 commits into
flutter:masterfrom
mpcomplete:text.label
Dec 14, 2016
Merged

mpcomplete merged 5 commits into
flutter:masterfrom
mpcomplete:text.label

Conversation

@mpcomplete

Copy link
Copy Markdown
Contributor

Fixes #7217

@HansMuller

Copy link
Copy Markdown
Contributor

LGTM

@Hixie

Hixie commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

This needs a test.
@HansMuller Please do not LGTM without a test. :-)

@mpcomplete Did you check how this looks? Often animating font-size looks terrible because of snapping in the font hinting, and we end up wanting to animate the transform instead.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

@Hixie yes, there's snapping (though I didn't notice it until I slowed down animations). That seems like a deficiency of AnimatedDefaultTextStyle. Can we fix that instead of introducing a transform here?

@Hixie

Hixie commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

What's going on is that fonts are not pure vectors, they're programs that snap to pixel boundaries based on their font size (this is known as hinting). So as you change the font size, the underlying vectors change in a non-linear fashion. Basically, animating the font size is not the same as animating the text growing.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

Right - but AnimatedDefaultTextStyle exists, and will happily animate between font sizes. We should figure out how to make that look good (or don't try, and explicitly note that it shouldn't be used for font size changes)

@Hixie

Hixie commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

Documenting it is certainly a good idea.

@mpcomplete

Copy link
Copy Markdown
Contributor Author

Here's another version that smoothly interpolates between the 2 font sizes using a Transform. Working on a test, but I wanted to get your feedback @Hixie to see if you liked this approach. I'm adding a scale param to DefaultTextStyle that Text uses to decide whether to add a Transform.

I also added a fade-in to the hintText, so it doesn't pop in immediately on top of the label while that animates out of the way.

@mpcomplete
mpcomplete requested a review from Hixie December 12, 2016 23:50

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this will only work for single-line text. There's not really a good way to do this for multiline text (which RichText in general supports).

@Hixie

Hixie commented Dec 12, 2016

Copy link
Copy Markdown
Contributor

I'm a little skeptical about adding a transform to RichText for this, because it will have really weird effects on multiline text.

@abarth may have another opinion, I'd be curious to hear what he thinks.

@abarth

abarth commented Dec 13, 2016

Copy link
Copy Markdown
Contributor

I don't think we should add a Transform to RichText. Instead, we should do something more specific to input field animations.

Also fade in the hintText so it doesn't pop in immediately as the label
text slides up.

Fixes #7217
@mpcomplete

Copy link
Copy Markdown
Contributor Author

OK, the label animation is now local to input.dart.

As for a test: this patch makes the animation "look better". That's not really something I can test via automation. I can test that there is now an animation, but that seems oddly specific and not really in the spirit of an automated test. WDYT?

@Hixie

Hixie commented Dec 13, 2016

Copy link
Copy Markdown
Contributor

What are you worried I'll accidentally regress when I walk into this code like a bull in a china shop?

Test that. :-)

@mpcomplete

Copy link
Copy Markdown
Contributor Author

Added a test.

@HansMuller

Copy link
Copy Markdown
Contributor

LGTM

@mpcomplete
mpcomplete merged commit bf289a2 into flutter:master Dec 14, 2016
@mpcomplete
mpcomplete deleted the text.label branch December 14, 2016 20:44
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Aug 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Text label doesn't scale up when focus is applied

5 participants