Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@GaryQian
Copy link
Contributor

@GaryQian GaryQian commented May 22, 2019

In order to better match the API description of TextStyle.height, this ensures that the height of the line is exactly fontSize * height logical pixels tall when a height is specified. When unspecified/omitted, the font's height will be used, which may be taller or shorter then the EM square.

This is a breaking change, even though it brings behavior back in line with the description.

Even though this changes a very fundamental API, it seems to not break as much as expected. Most implementations are not as dependent on the height property as I had suspected.

Fixes flutter/flutter#29479

@GaryQian
Copy link
Contributor Author

Still need to make the equivalent change for strut

@GaryQian GaryQian changed the title Implement TextStyle.height property as a multiple of font size instead of multiple of ascent+descent+leading. TextStyle.height property as a multiple of font size instead of multiple of ascent+descent+leading. May 22, 2019
@GaryQian GaryQian removed the Work in progress (WIP) Not ready (yet) for review! label May 23, 2019
@GaryQian GaryQian requested review from Hixie and jason-simmons May 23, 2019 19:38
Copy link
Member

@jason-simmons jason-simmons left a comment

Choose a reason for hiding this comment

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

Implementation LGTM

@GaryQian
Copy link
Contributor Author

GaryQian commented May 28, 2019

Waiting a bit before landing to give people an opportunity to comprehend and comment on this change. Breaking change announcement sent today.

@GaryQian
Copy link
Contributor Author

cc @tgrikas

@GaryQian
Copy link
Contributor Author

Waiting on current rolling status to clear up and become smoother before attempting this.

@Hixie
Copy link
Contributor

Hixie commented May 31, 2019

approach SGTM (it's what we talked about)
cc @goderbauer for review

Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM for the dart files

@cbracken
Copy link
Member

@GaryQian do you plan to rebase and merge this PR?

@GaryQian
Copy link
Contributor Author

Update: Still in my queue, dealing with some other crasher issues that came up in between. Should be able to get back to this soon.

@GaryQian
Copy link
Contributor Author

Update: Holding off on committing this until the tree is in a position to take riskier changes. Due to the very breaking nature of this change, the delay will allow for a much more flexible and longer period of time to work with breakages that this causes.

@GaryQian
Copy link
Contributor Author

GaryQian commented Jul 8, 2019

New diagram: flutter/assets-for-api-docs#90

@GaryQian GaryQian merged commit 75387db into flutter:master Jul 8, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 8, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 9, 2019
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 9, 2019
@tvolkert
Copy link
Contributor

Can you post a link to the breaking change announcement that was sent out for this (and also add the announcement link to the changelog)?

This is also listed in the changelog as being in v1.8.1, but it looks like it's not included in v1.8.1.

@GaryQian
Copy link
Contributor Author

Yes, here is the announcement. https://groups.google.com/forum/?utm_medium=email&utm_source=footer#!msg/flutter-announce/ZmnseDOW9Wc/5K7xD0V8BwAJ

I may have put it under the wrong z version number, I'll fix it! The dev release it is in is still under review.

@tvolkert
Copy link
Contributor

Thanks! I updated the changelog accordingly.

For archeologists, this was rolled into the framework in flutter/flutter#35814

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextStyle.height behavior

7 participants