这是indexloc提供的服务，不要输入任何密码
Skip to content

Set upper limit on text scaling for AppBar.title #58094

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Jun 18, 2020
Merged

Set upper limit on text scaling for AppBar.title #58094

merged 12 commits into from
Jun 18, 2020

Conversation

perclasson
Copy link
Contributor

@perclasson perclasson commented May 27, 2020 •
edited
Loading

Description

To follow the Material spec the title of AppBars should have a upper limit of x1.34 text scaling to keep the visual hierarchy in an application. The deprecated property shouldCapTextScaleForTitle that is false by default, was added for backwards compatibility with internal tests. After this PR has merged, it will be changed to true by default in one PR, and then removed in one afterwards. So it is just temporary while this change rolls through and should not be used by external developers.

For example, having the font size of 18, is 24 with maximum text scaling.

AppBar without text scaling:
Simulator Screen Shot - iPhone 11 Pro Max - 2020-05-27 at 14 37 59

With text scaling, but capped at 1.34:
Simulator Screen Shot - iPhone 11 Pro Max - 2020-05-27 at 14 38 03

Related Issues

Closes #58093

Tests

I added the following tests:

  • AppBars title has upper limit on text scaling, textScaleFactor = 1, 1.34, 2

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • [X ] I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.

@fluttergithubbot fluttergithubbot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 27, 2020
@perclasson perclasson changed the title Set upper limit on AppBar.title with text scaling Set upper limit on text scaling for AppBar.title May 27, 2020
guidezpl
guidezpl reviewed May 27, 2020
View reviewed changes
guidezpl
guidezpl reviewed May 27, 2020
View reviewed changes
guidezpl
guidezpl approved these changes May 27, 2020
View reviewed changes
Copy link
Member

@guidezpl guidezpl left a comment

Choose a reason for hiding this comment

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

██╗      ██████╗ ████████╗███╗   ███╗
██║     ██╔════╝ ╚══██╔══╝████╗ ████║
██║     ██║  ███╗   ██║   ██╔████╔██║
██║     ██║   ██║   ██║   ██║╚██╔╝██║
███████╗╚██████╔╝   ██║   ██║ ╚═╝ ██║
╚══════╝ ╚═════╝    ╚═╝   ╚═╝     ╚═╝

rami-a
rami-a approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM

@perclasson perclasson force-pushed the app_bar_max_text_font_size branch from 141c0ed to 98d7922 Compare May 28, 2020 09:32
@perclasson perclasson requested a review from HansMuller May 28, 2020 13:10
HansMuller
HansMuller approved these changes Jun 3, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM so long as it really doesn't break any existing internal or external tests.

packages/flutter/lib/src/material/app_bar.dart
@@ -554,6 +555,22 @@ class _AppBarState extends State<AppBar> {
overflow: TextOverflow.ellipsis,
child: title,
);

// Set maximum text scale factor to [_kMaxTitleTextScaleFactor] for the
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't help developers reading the API docs however it's an odd enough corner case that I think it's OK to leave it as it is - a private implementation comment.

@perclasson perclasson force-pushed the app_bar_max_text_font_size branch from f03d9eb to 9d7d260 Compare June 17, 2020 21:14
HansMuller
HansMuller approved these changes Jun 17, 2020
View reviewed changes
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

LGTM

The PRs description should explain what you're doing with the deprecated, default false, shouldCapTextScaleForTitle, property. Remind developers that it's just temporary while we roll this change through.

@perclasson perclasson force-pushed the app_bar_max_text_font_size branch from 9d7d260 to dbd8c60 Compare June 18, 2020 11:43
rami-a
rami-a approved these changes Jun 18, 2020
View reviewed changes
Copy link
Contributor

@rami-a rami-a left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite hostonly_devicelab_tests-2-linux has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot merged commit dbc6dca into flutter:master Jun 18, 2020
zljj0818 pushed a commit to zljj0818/flutter that referenced this pull request Jun 22, 2020
@perclasson @zljj0818
Set upper limit on text scaling for AppBar.title (flutter#58094)
00de03d
@perclasson perclasson mentioned this pull request Jul 7, 2020
Merged
10 tasks
mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020
@perclasson @mingwandroid
Set upper limit on text scaling for AppBar.title (flutter#58094)
153a330
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@HansMuller HansMuller HansMuller approved these changes

@rami-a rami-a rami-a approved these changes

@guidezpl guidezpl guidezpl approved these changes

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The title of AppBar's text scaling should be capped at x1.34 to keep the visual hierarchy
6 participants
@perclasson @fluttergithubbot @HansMuller @rami-a @guidezpl @googlebot