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

Animate size of label text over text fields. #7241

Merged
merged 5 commits into from
Dec 14, 2016
Merged

Animate size of label text over text fields. #7241

merged 5 commits into from
Dec 14, 2016

Conversation

mpcomplete
Copy link
Contributor

Fixes #7217

@HansMuller
Copy link
Contributor

LGTM

@Hixie
Copy link
Contributor

Hixie commented Dec 12, 2016

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
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
Copy link
Contributor

Hixie commented Dec 12, 2016

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
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
Copy link
Contributor

Hixie commented Dec 12, 2016

Documenting it is certainly a good idea.

@mpcomplete
Copy link
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
@@ -184,6 +189,13 @@ class Text extends StatelessWidget {
text: data
)
);
if (scale != 1.0) {
Copy link
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
Copy link
Contributor

Hixie commented Dec 12, 2016

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
Copy link
Contributor

abarth commented Dec 13, 2016

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
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
Copy link
Contributor

Hixie commented Dec 13, 2016

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
Contributor Author

Added a test.

@HansMuller
Copy link
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