[CupertinoActionSheet] Fix padding and font size of buttons #151199
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
github-actions
bot
added
framework
dkwingsmt
changed the title
dkwingsmt
force-pushed
the
as-text
branch
3 times, most recently
from
b5d8582 to
2c99637
Compare
| const TextStyle _kActionSheetActionStyle = TextStyle( | ||
| fontFamily: 'CupertinoSystemText', | ||
| // The fontSize and fontWeight may be adjusted when the text is rendered. | ||
| fontFamily: 'CupertinoSystemDisplay', |
There was a problem hiding this comment.
iOS always uses the Display font for font size >= 20, and according to the table noted in the comment later, the action sheet buttons always have size >= 20.
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| // For mid-sized text, an interpolated curve is used. The breakpoint for | ||
| // switching between these behaviors is 24.5 instead of 28 to allow closer | ||
| // interpolation over a smaller range. | ||
| final double resultSize = -18.8 + 3.68 * contextBodySize * (1 - 0.02128 * contextBodySize); |
There was a problem hiding this comment.
Seems like a lot of magic number. can you declare const in this method with appropriate name for readability?
There was a problem hiding this comment.
I agree, but I don't know how. Because this curve really is just interpolated. Let me ask around how people feel about it.
There was a problem hiding this comment.
can you explain a bit more how do you come up with this polynomial?
There was a problem hiding this comment.
Sure!
- I used the table above, and excluded the high end and the low end because they have simple rules, which leaves this:
// Text scale | l | xl | xxl | xxxl | ax1
// Body font | 17 | 19 | 21 | 23 | 28
// Button font | 21 | 23 | 24 | 24 | 28
- I cut the break point on the high end closer for better interpolation
// Text scale | l | xl | xxl | xxxl | --
// Body font | 17 | 19 | 21 | 23 | 24.5
// Button font | 21 | 23 | 24 | 24 | 24.5
- I sent this table to Google Sheet -> Chart -> Trend Line -> Cubit and got the result formula.
There was a problem hiding this comment.
I see, yeah In that case, there is probably not much to explain in the code.
It would be good to mention this polynomial is generated though interpolation tool.
There was a problem hiding this comment.
curious is the output not normalized as ax^2 + bx + c? The current format makes me think this is hand calculated, that is why I was wondering if you can define const to improve readability
There was a problem hiding this comment.
I plotted the original interpolation function on Desmos; unfortunately it doesn't line up exactly.
I discovered that the function
(translated to Dart code)
double _buttonFontSize(double contextBodySize) {
const double minSize = 21.0;
const double lowerBound = 17.0;
const double upperBound = 25.0;
if (contextBodySize <= lowerBound) {
return minSize;
}
if (contextBodySize >= upperBound) {
return contextBodySize;
}
final double x = contextBodySize - lowerBound;
// quadratic interpolation
return minSize + x * x / 16;
}There was a problem hiding this comment.
@nate-thegrate Yes, the ends don't align exactly, but since there's a roundToDouble at the end the fractional part doesn't matter.
For your second curve, beautiful and smooth as it is, it doesn't match the data points as noted in the doc. (I know, I wish it was a beautiful curve like this)
There was a problem hiding this comment.
curious is the output not normalized as ax^2 + bx + c?
Yeah you're right. I thought the current form would save a multiplication... but it's only useful for higher orders.
There was a problem hiding this comment.
@nate-thegrate I updated the doc and used the variable names in your doc! Thanks for the suggestion!
| const double higLargeBodySize = 17.0; | ||
| final double contextBodySize = MediaQuery.textScalerOf(context).scale(higLargeBodySize); | ||
| final double contextScaleFactor = contextBodySize / higLargeBodySize; | ||
| final double fontSize = _buttonFontSize(contextBodySize); |
There was a problem hiding this comment.
ah so the issue is that buttonfont size doe not scale linear with contextBodySize? that is unfortunate...
nate-thegrate
left a comment
nate-thegrate
left a comment
There was a problem hiding this comment.
This is super impressive work, thanks for doing it!
Co-authored-by: Greg Price <[email protected]>
dkwingsmt
added
the
autosubmit
Roll Flutter from 5103d75 to 58068d8 (42 revisions) flutter/flutter@5103d75...58068d8 2024-07-12 [email protected] Reland: Move all Linux Moto G4 tests to mokey in staging (flutter/flutter#151654) 2024-07-12 [email protected] Update obsolete comment in InputDecorator test (flutter/flutter#151651) 2024-07-12 [email protected] Fix `TabBar` tab indicator stretch effect (flutter/flutter#150868) 2024-07-12 [email protected] Remove workaround for a bug in dart2wasm (flutter/flutter#151603) 2024-07-12 [email protected] [native_assets] Stop running link hooks in JIT mode (flutter/flutter#151534) 2024-07-12 [email protected] Roll `Switch.adaptive` changes into `CupertinoSwitch` (flutter/flutter#149465) 2024-07-11 [email protected] Unmark java11 tests as bringup:true (flutter/flutter#151612) 2024-07-11 [email protected] Add link to design document archive (flutter/flutter#151489) 2024-07-11 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Move all Linux Moto G4 tests to mokey in staging (#151608)" (flutter/flutter#151620) 2024-07-11 [email protected] Move all Linux Moto G4 tests to mokey in staging (flutter/flutter#151608) 2024-07-11 [email protected] docimports for API samples (flutter/flutter#151606) 2024-07-11 [email protected] docimports for flutter_goldens, flutter_localizations, flutter_web_plugins, fuchsia_remote_debug_protocol, integration_test (flutter/flutter#151271) 2024-07-11 [email protected] Re-enable debug canvaskit e2e tests. (flutter/flutter#151565) 2024-07-11 [email protected] Fix: Submenu anchor misaligned with child panel in web (Resolved #151081) (flutter/flutter#151294) 2024-07-11 [email protected] Replaced {@tool snippet} with {@tool dartpad} in CupertinoTabController (flutter/flutter#151272) 2024-07-11 [email protected] feat: Support overriding native endorsed plugins (flutter/flutter#137040) 2024-07-11 [email protected] expose keyboardType in DropdownMenu #150894 (flutter/flutter#150896) 2024-07-11 [email protected] docimports for flutter_driver (flutter/flutter#151267) 2024-07-11 [email protected] Add `TimeOfDay` comparison methods (flutter/flutter#151233) 2024-07-11 [email protected] Roll Flutter Engine from 6534fbf3c2c1 to 36dccf7bb25c (2 revisions) (flutter/flutter#151577) 2024-07-11 [email protected] Roll Flutter Engine from 1c23c8f64076 to 6534fbf3c2c1 (3 revisions) (flutter/flutter#151572) 2024-07-11 [email protected] Use correct locale for `CupertinoDatePicker` weekday (flutter/flutter#151494) 2024-07-10 [email protected] doc imports for enum values (flutter/flutter#151548) 2024-07-10 [email protected] Reland "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests"... but no longer upgrade module AGP version (flutter/flutter#151433) 2024-07-10 [email protected] Roll Packages from 14341d1 to ea35fc6 (16 revisions) (flutter/flutter#151556) 2024-07-10 [email protected] [CupertinoActionSheet] Fix padding and font size of buttons (flutter/flutter#151199) 2024-07-10 [email protected] Roll Flutter Engine from db2b45bea2c0 to 1c23c8f64076 (2 revisions) (flutter/flutter#151550) 2024-07-10 [email protected] Add docImports for flutter_test references (flutter/flutter#151175) 2024-07-10 [email protected] Mention not @-mentioning people in commit messages in tree hygiene (flutter/flutter#151487) 2024-07-10 [email protected] Roll Flutter Engine from 371db85f33d7 to db2b45bea2c0 (8 revisions) (flutter/flutter#151522) 2024-07-10 [email protected] fix heading level absorption, diagnostics; add tests and an a11y use-case (flutter/flutter#151421) 2024-07-10 [email protected] Roll Flutter Engine from 9d943eb2b37a to 371db85f33d7 (3 revisions) (flutter/flutter#151505) 2024-07-10 [email protected] Roll Flutter Engine from d3269d5855a7 to 9d943eb2b37a (5 revisions) (flutter/flutter#151495) 2024-07-09 [email protected] Update doc of `SemanticsProperties.identifier` (flutter/flutter#149915) 2024-07-09 [email protected] Clean up leaky test. (flutter/flutter#151131) 2024-07-09 [email protected] Roll pub packages (flutter/flutter#151492) 2024-07-09 [email protected] testAdd tests for stepper.controls_builder.0.dart (flutter/flutter#150669) 2024-07-09 [email protected] Add Semantics Property `linkUrl` (flutter/flutter#150639) 2024-07-09 [email protected] Roll Flutter Engine from 4a2ac0e51a8f to d3269d5855a7 (1 revision) (flutter/flutter#151488) 2024-07-09 [email protected] Link engine docs on AS setup in flutter/flutter docs on engine contributor setup (flutter/flutter#151481) 2024-07-09 [email protected] Roll Flutter Engine from 69075e7e87d4 to 4a2ac0e51a8f (21 revisions) (flutter/flutter#151482) 2024-07-09 [email protected] Fix Material 3 `Dialog` default background color (flutter/flutter#151400) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages ...
4 participants
This PR updates the padding and font size of
CupertinoActionSheet's buttons to match native.The rules are derived from measuring on simulators:
Comparison
(Left to right: Native, Flutter after PR, Flutter before PR. The number at the bottom is the system text scale level, from 1 being extra small to 12 being accessibility large 5.)
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.