Skip to content

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

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
auto-submit merged 4 commits into from
Feb 21, 2025
Merged

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

auto-submit merged 4 commits into from
Feb 21, 2025

Conversation

@dickermoshe
Copy link

@dickermoshe dickermoshe commented Nov 4, 2024 •
edited
Loading

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. f: material design flutter/packages/flutter/material repository. labels Nov 4, 2024
@Piinks Piinks requested a review from nate-thegrate November 6, 2024 23:17
@justinmc
Copy link
Contributor

justinmc commented Nov 7, 2024

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

@dickermoshe
Copy link
Author

dickermoshe commented Nov 7, 2024 via email

@dickermoshe
Copy link
Author

dickermoshe commented Nov 8, 2024 •
edited
Loading

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

@nate-thegrate nate-thegrate left a comment

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

nate-thegrate commented Nov 10, 2024

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

dickermoshe commented Nov 11, 2024 •
edited
Loading

@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
@nate-thegrate

This comment was marked as resolved.

Sign in to view
@dickermoshe
Copy link
Author

dickermoshe commented Nov 12, 2024

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

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

@nate-thegrate
Copy link
Contributor

nate-thegrate commented Nov 13, 2024

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

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

@Renzo-Olivares Renzo-Olivares left a comment

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

Renzo-Olivares commented Nov 13, 2024

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

dickermoshe commented Dec 11, 2024

Hmmm, any update on this?

@Renzo-Olivares
Copy link
Contributor

Renzo-Olivares commented Dec 11, 2024

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

@Renzo-Olivares Renzo-Olivares left a comment

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

dkwingsmt commented Jan 22, 2025

I've updated the PR with rebasing.

nate-thegrate
nate-thegrate reviewed Jan 27, 2025
View reviewed changes
packages/flutter/test/material/text_theme_test.dart
Copy link
Contributor

@nate-thegrate nate-thegrate Jan 27, 2025

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

Piinks commented Feb 5, 2025

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

@dkwingsmt
Copy link
Contributor

dkwingsmt commented Feb 5, 2025

FYI running dart format packages/flutter/ should work.

@dickermoshe
Copy link
Author

dickermoshe commented Feb 5, 2025

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 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.

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

6 participants

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