Sitelet https://github.com/flutter/flutter/pull/158103
Skip to content

Adds support for applying delta/factor transformations for TextTheme height, letter and word spacing - #158103

Merged
auto-submit[bot] merged 4 commits into
flutter:masterfrom
dickermoshe:master
Feb 21, 2025
Merged

Adds support for applying delta/factor transformations for TextTheme height, letter and word spacing#158103
auto-submit[bot] merged 4 commits into
flutter:masterfrom
dickermoshe:master

Conversation

@dickermoshe

@dickermoshe dickermoshe commented Nov 4, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #158102

The text theme has the apply method which does bulk operations on multiple text styles. It supports delta/factor ajustements for font size. This is very helpful for changing all the font sizes at once using a ratio or a simple delta.
This PR add support for height, letter spacing and and word spacing too.

Why is this so useful?

Adjusting these in bulk is really helpful for using custom fonts. The Material font comes which its own default text styes and they're usually great. But many times they need to be nudged tighter.

Doc Comment

apply has no doc comments for fontSizeFactor/fontSizeDelta so i did not add any for the new letterSpacingFactor, letterSpacingDelta... either. If we want to add it, I'll do it.

Tests

Done!

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions Bot added a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages labels Nov 4, 2024
@Piinks
Piinks requested a review from nate-thegrate November 6, 2024 23:17
@justinmc

justinmc commented Nov 7, 2024

Copy link
Copy Markdown
Contributor

Thanks for the PR! Can you undo the formatting changes here to make it easier to review?

@dickermoshe

dickermoshe commented Nov 7, 2024 via email

Copy link
Copy Markdown
Contributor Author

@dickermoshe

dickermoshe commented Nov 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Can you undo the formatting changes here to make it easier to review?

Done

nate-thegrate
nate-thegrate approved these changes Nov 10, 2024
View reviewed changes

@nate-thegrate nate-thegrate left a comment

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.

Including all the TextStyle.apply() parameters in TextTheme.apply() sounds to me like a no-brainer 👍

LGTM, thanks for the contribution!

@nate-thegrate

This comment was marked as resolved.

Sign in to view
@nate-thegrate

nate-thegrate commented Nov 10, 2024

Copy link
Copy Markdown
Contributor

In the future, what can we contributors do to ensure that formatting is applied the same way the flutter repo expects it to.

In the future, we'll likely be adopting the new autoformatter! But until then, it'd probably be good to turn the formatter off in your IDE while editing files in this repository.

@dickermoshe

dickermoshe commented Nov 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@nate-thegrate

Problem

Seems there are some inconsistencies how TextStyle.apply() handles deltas/offsets when the original value is null.

Applying it to a height which is null is a no-op.
Applying it to a letterSpacing or wordSpacing which is null, throws an assert.

I wish this wasn't the case.

Semi-Solution

To avoid this I've done the following:

  • All the default typographies (2018-2021) have a letterSpacing for each style so no exceptions are thrown when delta/factoring them.
  • Only the Material 2021 typography has default heights. So to test the factor/delta on heights, I've modified the tests to use the Material 2021 defaults.
  • None of the defaults typographies have any wordSpacing defaults, so I hardcoded those to get the tests working.

All tests pass now.

Best Solution

IMHO, we should remove this inconstancy by either removing the assert from letterSpacing and wordSpacing (no breaking changes) or add an assert for height too (breaking change)

nate-thegrate
nate-thegrate reviewed Nov 11, 2024
View reviewed changes
Comment thread packages/flutter/test/material/text_theme_test.dart Outdated
@nate-thegrate

This comment was marked as resolved.

Sign in to view
@dickermoshe

dickermoshe commented Nov 12, 2024

Copy link
Copy Markdown
Contributor Author

I removed all the merge commits and performed a single rebase.

btw, feel free to modify my branch for such purposes.
image

@nate-thegrate

nate-thegrate commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

@Renzo-Olivares any idea what's going on with Google testing?

Renzo-Olivares
Renzo-Olivares approved these changes Nov 13, 2024
View reviewed changes

@Renzo-Olivares Renzo-Olivares left a comment

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.

LGTM, thanks for the contribution!

@Renzo-Olivares

Renzo-Olivares commented Nov 13, 2024

Copy link
Copy Markdown
Contributor

@nate-thegrate This one is looking like a breaking change for all users that override TextTheme.apply because they have to add these new parameters to their apply method. Going to double-check that's all, and if it is then i'll try to get google testing passing to get this landed.

@dickermoshe

dickermoshe commented Dec 11, 2024

Copy link
Copy Markdown
Contributor Author

Hmmm, any update on this?

@Renzo-Olivares

Renzo-Olivares commented Dec 11, 2024

Copy link
Copy Markdown
Contributor

Hi @dickermoshe, sorry for the delay. I’ll be taking a look this week.

Renzo-Olivares
Renzo-Olivares requested changes Jan 10, 2025
View reviewed changes

@Renzo-Olivares Renzo-Olivares left a comment

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.

@dickermoshe Thank you for your patience on this PR. Could you rebase the PR to the latest? There has been lots of changes (monorepo/autoformatter) since I last visited this PR. I have also gone through the google testing and fixed the issue, once the rebase is done we can move forward with merging this.

@dkwingsmt

dkwingsmt commented Jan 22, 2025

Copy link
Copy Markdown
Contributor

I've updated the PR with rebasing.

nate-thegrate
nate-thegrate reviewed Jan 27, 2025
View reviewed changes
Comment thread packages/flutter/test/material/text_theme_test.dart

@nate-thegrate nate-thegrate Jan 27, 2025

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.

Looks like Linux Analyze failed since this file doesn't match the auto-formatter.

@Piinks

Piinks commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

Hey @dickermoshe would you like to return to this change? The analyzer check is failing due to formatting.

@dkwingsmt

dkwingsmt commented Feb 5, 2025

Copy link
Copy Markdown
Contributor

FYI running dart format packages/flutter/ should work.

@dickermoshe

dickermoshe commented Feb 5, 2025

Copy link
Copy Markdown
Contributor Author

It's a lot of files, stand by...

@github-actions github-actions Bot added a: animation Animation APIs a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) f: scrolling Viewports, list views, slivers, etc. labels Feb 5, 2025
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
7c758c1 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
3256632 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
87fd38a 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 24, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
91ba006 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 24, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
b6408d7 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
c7dc3ec 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
a2f5bde 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
6b99304 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
25e54b4 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
fff968b 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Feb 25, 2025
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 25, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
6b29594 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
699e512 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
014e76f 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
c6ddcf0 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
fa81722 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
c3cec1c 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 26, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
9bed3e2 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
6324079 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
5bd1b6d 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
@engine-flutter-autoroll
4d50910 Adds support for applying delta/factor transformations for Te…
f901594 
…xtTheme height, letter and word spacing (flutter/flutter#158103)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Renzo-Olivares Renzo-Olivares Renzo-Olivares approved these changes
@dkwingsmt dkwingsmt dkwingsmt approved these changes
+1 more reviewer
@nate-thegrate nate-thegrate nate-thegrate approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

a: text input Entering text in a text field or keyboard related problems framework flutter/packages/flutter repository. See also f: labels. p: material_ui material_ui package in flutter/packages

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for applying delta/factor transformations for TextTheme height, letter and word spacing

6 participants

@dickermoshe @justinmc @nate-thegrate @Renzo-Olivares @dkwingsmt @Piinks