-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Fix FlexibleSpaceBar title color, alignment without the leading widget, & update tests for M3
#138611
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.
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
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
…get, & update tests for M3
|
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. |
|
Pushing an empty cmmit fixed this issue #138611 (comment). |
|
If landed, this might be worth CP. #132573 (comment) |
I have not finished reviewing this yet, but currently I don't think it's a good candidate for CP. It is a large change and fixes multiple issues instead of just one. If there is one issue in particular that needs a CP fix, we should separate it out. If all three issues are CP worthy, they should be broken out individually. 👍 |
|
That makes sense |
| bool hasLeading = leading != null; | ||
| if (leading == null && automaticallyImplyLeading) { | ||
| hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've probably spent too much time seeing how this could be reworked. I've tripped over it though every time I've read it. 😆
| bool hasLeading = leading != null; | |
| if (leading == null && automaticallyImplyLeading) { | |
| hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false); | |
| } | |
| bool hasLeading = leading != null || automaticallyImplyLeading; | |
| if (!hasLeading) { | |
| hasLeading = (scaffold?.hasDrawer ?? false) || (parentRoute?.impliesAppBarDismissal ?? false); | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, this is bit complicated but thanks to your effort with this suggestion, I've spotted a big issue with center alignment when leading widget is provided and spent few hours trying figure out what's going wrong here.
To make sure ALL the possible combinations are as expected. I created an ultimate code sample and added all configurations of FlexibleSpaceBar.title.
As you can see leading widgets mis-aligns the title, this was good for a long title fix #132573, it's not good for a regular title.
Unfortunately, this isn't possible to fix with with just padding as it will move the title position.
I've an idea to do something similar to _ExpandedTitleWithPadding for medium and large title.
flutter/packages/flutter/lib/src/material/app_bar.dart
Lines 2135 to 2142 in d49dcaa
| // A widget that bottom-start aligns its child (the expanded title widget), and | |
| // insets the child according to the specified padding. | |
| // | |
| // This widget gives the child an infinite max height constraint, and will also | |
| // attempt to vertically limit the child's bounding box (not including the | |
| // padding) to within the y range [0, maxExtent], to make sure the child is | |
| // visible when the AppBar is fully expanded. | |
| class _ExpandedTitleWithPadding extends SingleChildRenderObjectWidget { |
I'll figure out this for good and update the PR soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
I definitely misspoke here. I was collecting up all my PRs for review today and mixed this up with another - the code size is from all of the excellent tests. |
|
Closing to do some native testing, dig deeper into how Android itself handles this (I've this done this for some other widgets, it may also require updating docs and more testing than this PR has) and take feedback from Discord into account. |
fixes [Invisible SliverAppBar title in Material 3 light theme](#138296) fixes [`FlexibleSpaceBar` title is misaligned without the leading widget](#138608) Previous attempt #138611 --- ### Description - fixes the `FlexibleSpaceBar` centered title position when there is a leading widget. - fixes the `FlexibleSpaceBar` title color for Material 3. - Added documentation when using a long `FlexibleSpaceBar` title and update its test. - Improved documentation of default title padding. ### Code sample ### Code sample <details> <summary>expand to view the code sample</summary> ```dart import 'package:flutter/material.dart'; void main() => runApp(const MyApp()); class MyApp extends StatelessWidget { const MyApp({super.key}); @OverRide Widget build(BuildContext context) { return const MaterialApp( debugShowCheckedModeBanner: false, home: Example(), ); } } class Example extends StatelessWidget { const Example({super.key}); @OverRide Widget build(BuildContext context) { return const Scaffold( body: SafeArea( child: CustomScrollView( slivers: <Widget>[ SliverAppBar( leading: Icon(Icons.favorite_rounded), flexibleSpace: FlexibleSpaceBar( title: ColoredBox( color: Color(0xffff0000), child: Text('SliverAppBar'), ), ), ), ], )), ); } } ``` </details> ### Before  ### After 

fixes Invisible SliverAppBar title in Material 3 light theme
fixes
FlexibleSpaceBartitle is misaligned without the leading widgetDescription
FlexibleSpaceBarwidget for Material 3FlexibleSpaceBarwidget when leading widget isn't presentFlexibleSpaceBarwidget tests for Material 3Code sample
expand to view the code sample
Before
After
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.