-
Notifications
You must be signed in to change notification settings - Fork 7.7k
[Gallery] Add padding to demo appBar #289
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
Also can you add a screenshot for how it looks like on mobile? |
@@ -245,6 +247,7 @@ class _DemoPageState extends State<DemoPage> with TickerProviderStateMixin { | |||
_state == _DemoState.fullscreen ? selectedIconColor : iconColor, | |||
onPressed: () => _handleTap(_DemoState.fullscreen), | |||
), | |||
SizedBox(width: appBarPadding), |
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.
This would only solve the problem for the buttons on the right, but the back button on the left would still have the problem.
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.
Hence the SizedBox on the left :)
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.
Oh I see what you mean
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.
It looks like the back button is already afforded enough space for the ripple to show
This is the appBar for all demos, and sure! |
@guidezpl the issue is more evident in light theme but look at the ink ripple in the following screenshots |
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.
LGTM with one small change
gallery/gallery/lib/pages/demo.dart
Outdated
@@ -179,15 +179,19 @@ class _DemoPageState extends State<DemoPage> with TickerProviderStateMixin { | |||
final colorScheme = Theme.of(context).colorScheme; | |||
final iconColor = colorScheme.onSurface; | |||
final selectedIconColor = colorScheme.primary; | |||
final appBarPadding = 20.0; |
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.
Only do this if it is desktop, otherwise it'll look strange on mobile
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.
Good call
This adds a bit of padding after the actions for the last ripple to be visible
Closes material-components/material-components-flutter-gallery#208