Skip to content

Add TextLeadingDistribution to TextStyle #77001

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 9 commits into from
Mar 9, 2021
Merged

Add TextLeadingDistribution to TextStyle #77001

merged 9 commits into from
Mar 9, 2021

Conversation

@LongCatIsLooong
Copy link
Contributor

@LongCatIsLooong LongCatIsLooong commented Mar 1, 2021 •
edited
Loading

Engine PR: flutter/engine#24668

With minimal documentation. I'll expand the documentation later with images. Things to explain:

  • How alphabetic baselines are determined given a StrutStyle or TextStyle.
  • Mention how TextHeightBehavior's leading trim affects the line-height.

List which issues are fixed by this PR. You must list at least one issue.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt.
  • All existing and new tests are passing.

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

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Mar 1, 2021
@google-cla google-cla bot added the cla: yes label Mar 1, 2021
justinmc
justinmc reviewed Mar 4, 2021
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

The approach LGTM. I mentioned tests and docs in my comments but it seems like maybe you're planning to come back to that.

packages/flutter/test/painting/text_style_test.dart
Comment on lines +33 to +34
// TODO(LongCatIsLooong): web support for
// https://github.com/flutter/flutter/issues/72521
Copy link
Contributor

@justinmc justinmc Mar 4, 2021

Choose a reason for hiding this comment

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

Did you or MH come up with a plan on how to do this? Hopefully it will now map closely to what CSS does with leading and line-height.

Copy link
Contributor

@justinmc justinmc Mar 4, 2021

Choose a reason for hiding this comment

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

Oh and is it documented anywhere that leadingDistribution has no effect on web right now?

Copy link
Contributor

@justinmc justinmc Mar 4, 2021

Choose a reason for hiding this comment

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

Just noticed that you wrote that you plan to update the documentation later.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Mar 8, 2021

Choose a reason for hiding this comment

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

I think the canvaskit backend is now the default. Not sure how this can be done with the css backend. I'll come back to the documentation when I start working on the web part.

Copy link
Contributor

@justinmc justinmc Mar 8, 2021

Choose a reason for hiding this comment

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

Sounds good. Well it should be much easier on the canvaskit backend, and it should end up working closely to how CSS does it anyway since this half leading approach came from the CSS spec as I understand it.

packages/flutter/test/painting/text_style_test.dart
expect(
style.apply(leadingDistribution: TextLeadingDistribution.proportional).leadingDistribution,
TextLeadingDistribution.proportional,
);
Copy link
Contributor

@justinmc justinmc Mar 4, 2021

Choose a reason for hiding this comment

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

Can you actually test the effect of leadingDistribution in this PR?

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Mar 5, 2021

Choose a reason for hiding this comment

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

I tested that in the engine repo but doing basic e2e tests sounds like a good idea!

Copy link
Contributor

@justinmc justinmc Mar 8, 2021

Choose a reason for hiding this comment

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

Oh that's right 👍

@LongCatIsLooong LongCatIsLooong marked this pull request as draft March 5, 2021 17:59
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review March 8, 2021 08:38
justinmc
justinmc approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@justinmc justinmc left a comment

Choose a reason for hiding this comment

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

LGTM 👍

packages/flutter/test/painting/text_painter_test.dart Outdated
Copy link
Contributor

@justinmc justinmc Mar 8, 2021

Choose a reason for hiding this comment

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

Where does the 1 come from? I was expecting a 0.1 from the height.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Mar 8, 2021

Choose a reason for hiding this comment

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

Added to the comment that the height multiplier does not scale the glyph only the line-height.

@LongCatIsLooong LongCatIsLooong force-pushed the add-textstyle.textHeightBehavior branch from 46a651e to f4ee84a Compare March 8, 2021 18:09
@fluttergithubbot fluttergithubbot merged commit 8c009aa into flutter:master Mar 9, 2021
@LongCatIsLooong LongCatIsLooong deleted the add-textstyle.textHeightBehavior branch March 9, 2021 04:19
@LongCatIsLooong LongCatIsLooong mentioned this pull request Mar 13, 2021
Closed
Hixie
Hixie reviewed Mar 13, 2021
View reviewed changes
packages/flutter/test/painting/text_style_test.dart
import 'dart:ui' as ui show TextStyle, ParagraphStyle, FontFeature, Shadow;

import 'package:flutter/painting.dart';
import 'package:flutter/src/foundation/constants.dart';
Copy link
Contributor

@Hixie Hixie Mar 13, 2021

Choose a reason for hiding this comment

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

FWIW, it should just be enough to import package:flutter/foundation.dart

@goderbauer don't we have a lint that should have caught this? Or does it not apply to tests?

@HansMuller HansMuller mentioned this pull request Oct 25, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hixie Hixie Hixie left review comments

@justinmc justinmc justinmc approved these changes

@GaryQian GaryQian Awaiting requested review from GaryQian

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

@johnsonmh johnsonmh Awaiting requested review from johnsonmh

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.

4 participants

@LongCatIsLooong @justinmc @Hixie @fluttergithubbot