Fix: CupertinoActionSheet should take up max height when actions section is short
#150708
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bleroux
reviewed
Jun 24, 2024
Contributor
bleroux
left a comment
There was a problem hiding this comment.
While reading this PR for learning purposes, I spotted two small nits.
Co-authored-by: Bruno Leroux <bruno.leroux@gmail.com>
…o as-priority-column
dkwingsmt
commented
Jun 26, 2024
|
|
||
| void main() { | ||
| testWidgets('Overall looks correctly under light theme', (WidgetTester tester) async { | ||
| testWidgets('Overall appearance is correct for the light theme', (WidgetTester tester) async { |
Contributor
Author
There was a problem hiding this comment.
I sneaked in some test name fixes as suggested by Hans in #150725 (comment)
chunhtai
reviewed
Jul 1, 2024
Contributor
chunhtai
left a comment
There was a problem hiding this comment.
refactor looks good, just some question and nits
| // Both child widgets stretch horizontally to the parent's maximum width | ||
| // constraint, with vertical space allocated in this priority: | ||
| // | ||
| // 1. The `bottom` widget receives its requested height, up to a |
Contributor
There was a problem hiding this comment.
I can't parse this sentence. according to the code height <= bottomMinHeight.
should it be call bottomMaxHeight
Contributor
Author
There was a problem hiding this comment.
It might be easier to understand it from the other way.
- If both widgets fit in the container, great, render both in their intrinsic height.
- If not, then prioritize reducing the bottom widget's height.
- However, the bottom widget can not be reduced too much. It has a
minHeight. If the bottom widget has been reduced tominHeight, then further reduction has to come from the top widget.
- However, the bottom widget can not be reduced too much. It has a
| @override | ||
| double computeMinIntrinsicHeight(double width) { | ||
| assert(childCount == 2); | ||
| return firstChild!.getMinIntrinsicHeight(width) + lastChild!.getMinIntrinsicHeight(width); |
Contributor
There was a problem hiding this comment.
This should also roughly follow the layout algorithm, are we assume this widget will not be wrap in a intrinsicHeight widget?
Contributor
There was a problem hiding this comment.
should this just call _childHeight with infinite height constrant?
Contributor
Author
There was a problem hiding this comment.
Sorry, I may not be understanding computeMinIntrinsicHeight correctly. Can you explain more how you think it should change and why?
Contributor
There was a problem hiding this comment.
I originally thought the bottom will have min height, but looks like it can be as small as it can. sorry for the confusion, this looks fine.
sigurdm
pushed a commit
to sigurdm/flutter
that referenced
this pull request
Jul 2, 2024
…ction is short (flutter#150708) This PR addresses a bug from the previous rewrite of the `CupertinoActionSheet` height allocation algorithm. The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than `_kActionSheetActionsSectionMinHeight`, the remaining space will not be taken by the content section. As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget `_PriorityColumn` that uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!) Some other trivial refactor is also done to clean up the code around parameters of `_ActionSheetMainSheet`.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Jul 2, 2024
…tions section is short (flutter/flutter#150708)
auto-submit bot
pushed a commit
to flutter/packages
that referenced
this pull request
Jul 3, 2024
flutter/flutter@99bb2ff...af913a7 2024-07-02 andrewrkolos@gmail.com Use `ErrorHandlingFileSystem.deleteIfExists` when deleting .plugin_symlinks (flutter/flutter#151073) 2024-07-02 hans.muller@gmail.com ScrollEndNotification example: auto-scroll based on RenderSliver constraints and geometry (flutter/flutter#143538) 2024-07-02 engine-flutter-autoroll@skia.org Roll Packages from 412ec46 to d2705fb (13 revisions) (flutter/flutter#151169) 2024-07-02 goderbauer@google.com docimports for painting (flutter/flutter#151143) 2024-07-02 goderbauer@google.com docimports for scheduler (flutter/flutter#151126) 2024-07-02 nate.w5687@gmail.com `dismissible.dart` code cleanup (flutter/flutter#150276) 2024-07-02 goderbauer@google.com docimports for physics (flutter/flutter#151125) 2024-07-02 goderbauer@google.com docimports for services (flutter/flutter#151134) 2024-07-02 goderbauer@google.com docimports for cupertino (flutter/flutter#151149) 2024-07-02 goderbauer@google.com docimports for gestures (flutter/flutter#151123) 2024-07-02 goderbauer@google.com Docimports for foundation (flutter/flutter#151119) 2024-07-02 goderbauer@google.com docimports for semantics (flutter/flutter#151132) 2024-07-02 jonahwilliams@google.com [flutter_driver] add allocator mtl to memory event allowlist. (flutter/flutter#151153) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 40c087b31515 to 433d360eee11 (7 revisions) (flutter/flutter#151165) 2024-07-02 sigurdm@google.com Refactor BuildInfo to always require packageConfigPath (flutter/flutter#150559) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from d3c5bd66a78f to 40c087b31515 (1 revision) (flutter/flutter#151156) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from fc5bc14e6091 to d3c5bd66a78f (1 revision) (flutter/flutter#151155) 2024-07-02 dkwingsmt@users.noreply.github.com Fix: `CupertinoActionSheet` should take up max height when actions section is short (flutter/flutter#150708) 2024-07-02 engine-flutter-autoroll@skia.org Roll Flutter Engine from 3456fee1a6b9 to fc5bc14e6091 (8 revisions) (flutter/flutter#151150) 2024-07-02 andrewrkolos@gmail.com [tool] remove some temporary printTrace calls (flutter/flutter#151074) 2024-07-01 nate.w5687@gmail.com Implementing a few switch statements (flutter/flutter#150946) 2024-07-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (#150969)" (flutter/flutter#151147) 2024-07-01 34871572+gmackall@users.noreply.github.com Upgrade template Gradle, App AGP, Module AGP, and Kotlin versions, and tests (flutter/flutter#150969) 2024-07-01 engine-flutter-autoroll@skia.org Roll Flutter Engine from b57a044ed10f to 3456fee1a6b9 (5 revisions) (flutter/flutter#151127) 2024-07-01 codefu@google.com Read AndroidManifest.xml and emit manifest-aar-impeller-(enabled|disabled) analytics (flutter/flutter#150970) 2024-07-01 goderbauer@google.com More docimports for animation library (flutter/flutter#151011) 2024-07-01 goderbauer@google.com Bump dartdoc to 8.0.10 (flutter/flutter#151107) 2024-07-01 matthiasngeozhongsim@yahoo.com Fix missing `[` in docs (flutter/flutter#151091) 2024-07-01 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#151028) 2024-07-01 jason-simmons@users.noreply.github.com Fix teardown of a FocusScopeNode in material/app_test (flutter/flutter#151115) 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 Please CC camillesimon@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
victorsanni
pushed a commit
to victorsanni/flutter
that referenced
this pull request
Jul 8, 2024
…ction is short (flutter#150708) This PR addresses a bug from the previous rewrite of the `CupertinoActionSheet` height allocation algorithm. The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than `_kActionSheetActionsSectionMinHeight`, the remaining space will not be taken by the content section. As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget `_PriorityColumn` that uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!) Some other trivial refactor is also done to clean up the code around parameters of `_ActionSheetMainSheet`.
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Aug 6, 2024
…tions section is short (flutter/flutter#150708)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses a bug from the previous rewrite of the
CupertinoActionSheetheight allocation algorithm.The previous approach assigns the content section with a minimal height. As a result, if the actions section is shorter than
_kActionSheetActionsSectionMinHeight, the remaining space will not be taken by the content section.As far as I know, this algorithm can not be implemented by simply compositing simple layout widgets, and therefore I created a new widget
_PriorityColumnthat uses a custom layout algorithm. (The resulting code is still shorter (and cleaner) than before the rewrite!)Some other trivial refactor is also done to clean up the code around parameters of
_ActionSheetMainSheet.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.