-
Notifications
You must be signed in to change notification settings - Fork 309
action_sheet: Redesign bottom sheet #853
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
8387fb7
to
3444df5
Compare
lib/widgets/action_sheet.dart
Outdated
showDraggableScrollableModalBottomSheet<void>( | ||
final designVariables = DesignVariables.of(context); | ||
showModalBottomSheet<void>( |
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.
For the current behavior of the action sheet (mentioned in #90 comment), there is no need to use showDraggableScrollableModalBottomSheet
.
As a side note, _DragHandleLayer
is no longer necessary in showDraggableScrollableModalBottomSheet
as there is showDragHandle
property available in showModalBottomSheet
. Also, showDraggableScrollableModalBottomSheet
is no longer used in the codebase, so I am not sure if it is meant to stay there for future usage.
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.
Yeah, a follow-up commit in this PR can remove that function — that'd be a nice cleanup, thanks. I expect we'll want all our bottom sheets to work the same way as this one.
And if we do want to go back and use that implementation for something, it's always there in the Git history.
As a side note,
_DragHandleLayer
is no longer necessary inshowDraggableScrollableModalBottomSheet
as there isshowDragHandle
property available inshowModalBottomSheet
.
True. It looks like that was added in flutter/flutter#122445, back in 2023-03… and both @chrisbobbe and I actually reviewed it (mainly on the predecessor thread flutter/flutter#120855). That landed after we merged Chris's implementation here, in #12.
I don't recall if that upstream implementation had behavior differences that would have caused us to stick with ours. I suspect we just didn't bother to switch to it, having already moved on to other tasks by the time it landed upstream.
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 don't recall if that upstream implementation had behavior differences that would have caused us to stick with ours. I suspect we just didn't bother to switch to it, having already moved on to other tasks by the time it landed upstream.
Yes, I investigated some months ago and there was a behavior difference that caused us to stick with ours. Here's the context around where we use _DragHandleLayer
:
builder: (BuildContext context) {
// Make the content start below the drag handle in the y-direction, but
// when the content is scrollable, let it scroll under the drag handle in
// the z-direction.
return Stack(
children: [
_DraggableScrollableLayer(builder: builder),
_DragHandleLayer(),
]);
});
If we migrated to showDragHandle: true
, then we'd have to give up the part after "but" in that comment. You can simulate this by making there be more than a screenful of options in the sheet:
Current | Migrated to showDragHandle: true |
---|---|
![]() |
![]() |
The culprit is a Padding
in upstream that always forces the caller's content to begin below the drag-handle area in the y-direction:
code
Widget bottomSheet = Material(
key: _childKey,
color: color,
elevation: elevation,
surfaceTintColor: surfaceTintColor,
shadowColor: shadowColor,
shape: shape,
clipBehavior: clipBehavior,
child: NotificationListener<DraggableScrollableNotification>(
onNotification: extentChanged,
child: !showDragHandle
? widget.builder(context)
: Stack(
alignment: Alignment.topCenter,
children: <Widget>[
dragHandle!,
Padding(
padding: const EdgeInsets.only(top: kMinInteractiveDimension),
child: widget.builder(context),
),
],
),
),
);
I did mention our desired behavior in my review of that upstream work that Greg mentioned, but didn't follow up when it turned out not to be supported.
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 does turn out to be moot for us, though, since the redesigned sheet doesn't have a drag handle.)
3444df5
to
664fce5
Compare
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.
Thanks for working on this @sm-sayedi.
Added comments for some things I've noticed to be different from the linked Figma design.
In addition to those, the Icons also need to be updated to match Figma design. #90 (comment) mentions to do it after #200 is done, and now it's done, so I think they should be updated and any new ones should be added in ZulipIcons
.
lib/widgets/action_sheet.dart
Outdated
style: MenuItemButton.styleFrom( | ||
backgroundColor: designVariables.actionSheetMenuButtonBackground, | ||
foregroundColor: designVariables.actionSheetMenuButtonForeground, | ||
), |
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.
nit indentation:
style: MenuItemButton.styleFrom( | |
backgroundColor: designVariables.actionSheetMenuButtonBackground, | |
foregroundColor: designVariables.actionSheetMenuButtonForeground, | |
), | |
style: MenuItemButton.styleFrom( | |
backgroundColor: designVariables.actionSheetMenuButtonBackground, | |
foregroundColor: designVariables.actionSheetMenuButtonForeground, | |
), |
lib/widgets/action_sheet.dart
Outdated
style: MenuItemButton.styleFrom( | ||
backgroundColor: designVariables.actionSheetMenuButtonBackground, |
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.

Let's override default padding and specify it explicitly, the suggested values here are adapted from Figma design:
style: MenuItemButton.styleFrom( | |
backgroundColor: designVariables.actionSheetMenuButtonBackground, | |
style: MenuItemButton.styleFrom( | |
padding: const EdgeInsets.fromLTRB(16, 12, 16, 12), | |
backgroundColor: designVariables.actionSheetMenuButtonBackground, |
lib/widgets/action_sheet.dart
Outdated
onPressed: () => onPressed(context), | ||
child: Text(label(zulipLocalizations))); | ||
child: Text(label(zulipLocalizations), style: const TextStyle(fontSize: 16))); |
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.
lib/widgets/action_sheet.dart
Outdated
), | ||
onPressed: () => Navigator.pop(context), | ||
child: Text(ZulipLocalizations.of(context).dialogCancel, | ||
style: const TextStyle(fontSize: 16)), |
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.
Same for this, text style needs to match Figma design.
lib/widgets/action_sheet.dart
Outdated
return ElevatedButton( | ||
style: ElevatedButton.styleFrom( | ||
elevation: 0, | ||
backgroundColor: designVariables.actionSheetCancelButtonBackground, | ||
foregroundColor: designVariables.actionSheetCancelButtonForeground, | ||
shadowColor: Colors.transparent, | ||
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), | ||
), |
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.
Can use TextButton
here:
return ElevatedButton( | |
style: ElevatedButton.styleFrom( | |
elevation: 0, | |
backgroundColor: designVariables.actionSheetCancelButtonBackground, | |
foregroundColor: designVariables.actionSheetCancelButtonForeground, | |
shadowColor: Colors.transparent, | |
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), | |
), | |
return TextButton( | |
style: TextButton.styleFrom( | |
backgroundColor: designVariables.actionSheetCancelButtonBackground, | |
foregroundColor: designVariables.actionSheetCancelButtonForeground, | |
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)), | |
), |
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.
lib/widgets/action_sheet.dart
Outdated
), | ||
), | ||
), | ||
const MessageActionSheetCancelButton(), |
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.
lib/widgets/action_sheet.dart
Outdated
if (!hasThumbsUpReactionVote) AddThumbsUpButton(message: message, messageListContext: context), | ||
StarButton(message: message, messageListContext: context), | ||
if (isComposeBoxOffered) QuoteAndReplyButton( | ||
message: message, | ||
messageListContext: context, | ||
), | ||
CopyMessageTextButton(message: message, messageListContext: context), | ||
CopyMessageLinkButton(message: message, messageListContext: context), | ||
ShareButton(message: message, messageListContext: context), |
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.
The order doesn't match the Figma design, it should be:
AddThumbsUpButton(),
CopyMessageTextButton(),
CopyMessageLinkButton(),
ShareButton(),
StarButton(),
QuoteAndReplyButton(),
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.
Yeah, it is different. But Alya decided to keep it as it is. 🙂
lib/widgets/action_sheet.dart
Outdated
message: message, | ||
messageListContext: context, | ||
), | ||
CopyMessageTextButton(message: message, messageListContext: context), |
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.
CopyMessageTextButton
's text needs to be changed to "Copy to clipboard".
lib/widgets/action_sheet.dart
Outdated
), | ||
CopyMessageTextButton(message: message, messageListContext: context), | ||
CopyMessageLinkButton(message: message, messageListContext: context), | ||
ShareButton(message: message, messageListContext: context), |
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.
ShareButton
's text should be changed to "Share link"
e798189
to
50e8b68
Compare
Thanks @rajveermalviya for the detailed review! It was really helpful. Revision pushed. PTAL. Also added a comment in the previous sub-thread. |
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.
Thanks for the revision @sm-sayedi, this looks good to me, just one comment.
Moving on to the mentor review from @hackerkid.
lib/widgets/action_sheet.dart
Outdated
onPressed: () => onPressed(context), | ||
child: Text(label(zulipLocalizations))); | ||
child: Text(label(zulipLocalizations), | ||
style: const TextStyle(fontSize: 20, fontWeight: FontWeight.w600), |
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 think fontWeight is not being applied correctly, you need to use weightVariableTextStyle
:
style: const TextStyle(fontSize: 20, fontWeight: FontWeight.w600), | |
style: const TextStyle(fontSize: 20) | |
.merge(weightVariableTextStyle(context, wght: 600)), |
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.
Similarly for the Cancel button.
lib/widgets/action_sheet.dart
Outdated
// TODO: The following line could be replaced by the [spacing] | ||
// property when https://github.com/flutter/flutter/issues/55378 is fixed. | ||
].expand((item) => [const SizedBox(height: 1), item]).skip(1).toList()), |
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.
Now that the mentioned upstream issue is fixed, we can simply add spacing: 1
to Column
. I will do that after #872 is merged.
50e8b68
to
f28a083
Compare
Thanks @rajveermalviya for the tip. I was confused about why the weight didn't have any effect. I thought there are not enough variations of the font available in the assets folder. |
505931d
to
f19b211
Compare
9c07846
to
ccff5fb
Compare
Thanks @chrisbobbe for the review! Changes pushed. |
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.
Thanks for the revision!
I see your explanation at #853 (comment) about the FutureBuilder
, etc., and I think I've found some working code that seems much simpler, and also follows the Figma more closely.
In particular, I think the "dynamic padding" that you mention in some code comments isn't something we want. Here's the description from #853 (comment) of what I saw in the Figma, which I still think is accurate:
When the buttons area can scroll (when there are more buttons than fit on the user's screen), the content, as it scrolls, should be allowed to show through this 8px gap, but masked with a gradient from transparent to DesignVariables.bgContextMenu: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3483-42553&m=dev
(Same with an area of height 8px above the buttons.)
and some working code for that:
// Pad the bottom inset. The left/top/right insets are already handled by
// `showModalBottomSheet.useSafeArea: true` above, which keeps the sheet
// out of those insets.
return SafeArea(
minimum: const EdgeInsets.only(bottom: 16),
child: Padding(
padding: const EdgeInsets.fromLTRB(16, 8, 16, 0),
child: Column(
crossAxisAlignment: CrossAxisAlignment.stretch,
mainAxisSize: MainAxisSize.min,
children: [
// TODO(#217): show message text
Flexible(
child: Stack(
children: [
SingleChildScrollView(
padding: const EdgeInsets.symmetric(vertical: 8),
child: ClipRRect(
borderRadius: BorderRadius.circular(7),
child: Column(spacing: 1, children: optionButtons))),
Positioned(
top: 0, right: 0, left: 0,
child: Container(
height: 8,
decoration: BoxDecoration(
gradient: LinearGradient(
begin: Alignment.topCenter,
end: Alignment.bottomCenter,
colors: [
designVariables.bgContextMenu,
designVariables.bgContextMenu.withOpacity(0),
])))),
Positioned(
bottom: 0, right: 0, left: 0,
child: Container(
height: 8,
decoration: BoxDecoration(
gradient: LinearGradient(
begin: Alignment.topCenter,
end: Alignment.bottomCenter,
colors: [
designVariables.bgContextMenu.withOpacity(0),
designVariables.bgContextMenu,
])))),
])),
const MessageActionSheetCancelButton(),
])));
i.e.:
- letting the bottom of the scroll's viewport extend all the way to the top edge of the cancel button, but with 8px bottom padding on the content and an 8px gradient overlay stacked on top of that in the z-direction
- letting the top of the scroll's viewport extend as far as 8px from the top of the sheet, but with 8px top padding on the content and an 8px gradient overlay stacked on top of that in the z-direction
In my manual testing of your revision, with the SizedBox
whose height changes on the scroll extent, the vertical displacement didn't seem natural to me. If the scroll extent is small but nonzero, then when I drag my finger, the content doesn't track exactly with my finger position; IIRC it went a bit faster.
My proposed code could maybe be further refactored to deduplicate some code, but I think it's an improvement over the solution here, which is still complicated enough that I'm not sure I've really understood it. 🙂 If you'd still like to show your proposal to Greg (after his vacation), that's fine; in that case you can just include a version of my proposal in a followup commit without squashing it into the main commit.
lib/widgets/action_sheet.dart
Outdated
tapTargetSize: MaterialTapTargetSize.shrinkWrap, | ||
minimumSize: Size.zero, |
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.
Bump on removing these if they don't do anything: #853 (comment)
lib/widgets/action_sheet.dart
Outdated
scrollController: scrollController, | ||
builder: (_, scrollController) => SizedBox( | ||
height: math.max( | ||
16 - scrollController.position.extentBefore, |
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.
Bump on #853 (comment) (using 8 here instead of 16).
ccff5fb
to
f853989
Compare
Thanks @chrisbobbe for the review and the proposed approach! I prefer yours and there is no need for the "dynamic padding". |
aa286c7
to
ab74659
Compare
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.
Thanks! Small comments below.
Then about the shadow effect, I noticed that @PIG208 has implemented basically the same thing—take a look at _InsetShadowBox
in the current revision of #928. If that seems similar enough, it could be a nice opportunity to pull out a helper widget. (I wouldn't block on doing that, though; we can always do it as a followup to both PRs.)
Marking for @gnprice's review.
test/widgets/action_sheet_test.dart
Outdated
group('MessageActionSheetCancelButton', () { | ||
final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | ||
|
||
Finder cancelButtonFinder() => find.text(zulipLocalizations.dialogCancel); |
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.
Can this just be a Finder
, instead of a function that returns a Finder
?
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.
nit: also a good name would be findCancelButton
— that way it's parallel to the find.foo
names from flutter_test
upstream
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.
Can this just be a
Finder
, instead of a function that returns aFinder
?
It could be a direct Finder
but I think making it a function with a relevant name makes the code a little bit more readable. 🙂
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 think Chris's suggestion was to make it a local variable so it still has a relevant name — just not a function:
final findCancelButton = find.text(zulipLocalizations.dialogCancel);
lib/widgets/action_sheet.dart
Outdated
], | ||
), | ||
), | ||
const MessageActionSheetCancelButton(), | ||
], | ||
), | ||
), | ||
); |
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.
nit: indentation
The current implementation got a lot simpler. Because I realized that we always have some paddings such that the content does not get covered by the gradient (and thus the shadow is not visible without scrolling). It seems that we also have the paddings here, it should be a straightforward refactor to reuse the widget. That PR will probably land later than this one, so I opened #990 to make |
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.
Thanks @sm-sayedi, and thanks @rajveermalviya and @chrisbobbe for the previous reviews!
Generally this all looks great. For the shadows, I think #990 will merge soon; then you can rebase atop that, which I think will simplify this code somewhat. (I'll also do a bit of manual testing of that aspect of the UI at that stage.)
Other than that, small comments below, and Chris has a couple of small comments open at #853 (review) above.
Let's also squash the "delete the old way" commit into the main commit here. Because the new code is replacing the old, the comparison is helpful for understanding some aspects of what the new code is doing.
// Clip.hardEdge looks bad; Clip.antiAliasWithSaveLayer looks pixel-perfect | ||
// on my iPhone 13 Pro but is marked as "much slower": | ||
// https://api.flutter.dev/flutter/dart-ui/Clip.html | ||
clipBehavior: Clip.antiAlias, |
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 comment seems like it's just as relevant after this change as before. So it should get moved along with the clipBehavior: Clip.antiAlias,
line.
lib/widgets/action_sheet.dart
Outdated
@@ -431,3 +505,28 @@ class ShareButton extends MessageActionSheetMenuItemButton { | |||
} | |||
} | |||
} | |||
|
|||
class MessageActionSheetCancelButton extends StatelessWidget { |
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.
nit: Let's put this right after MessageActionSheetMenuItemButton
— there's a lot of similarities in their code, so it's helpful to see them at the same time.
test/widgets/action_sheet_test.dart
Outdated
group('MessageActionSheetCancelButton', () { | ||
final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | ||
|
||
Finder cancelButtonFinder() => find.text(zulipLocalizations.dialogCancel); |
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.
nit: also a good name would be findCancelButton
— that way it's parallel to the find.foo
names from flutter_test
upstream
ab74659
to
92a6ae8
Compare
Thanks @chrisbobbe and @gnprice for the reviews. Changes pushed. |
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.
Thanks for the revision! One nit below, and one open above at #853 (comment) .
Otherwise the code all looks good. I said above I'd also do a final bit of manual testing; I won't get to that today but will put it on my queue for early next week.
(I have had earlier versions of this PR deployed on my phone off and on over the last few weeks, and the UI has looked great.)
lib/widgets/action_sheet.dart
Outdated
child: Column(spacing: 1, children: optionButtons), | ||
)))), |
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.
nit:
child: Column(spacing: 1, children: optionButtons), | |
)))), | |
child: Column(spacing: 1, | |
children: optionButtons))))), |
- our standard close-paren formatting
- and perhaps more importantly: keeps
children: optionButtons
, which is the ultimate payload that contains the meaningful content which everything else here is wrapping, visible on its own line
92a6ae8
to
84f8eb6
Compare
Thanks for the review @gnprice. Changes applied, PTAL. |
These icons come from the "Icons" page in the Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=544-22131&t=tmBcUzngPhgtutIT-0
Also, remove the custom modal bottom sheet we no longer use. Fixes: zulip#90
All looks good; and I did that manual testing of the scrolling behavior, and that looks good too. Merging. Thanks again @sm-sayedi for all your work on this! |
84f8eb6
to
f8ddff2
Compare
Matches the design of the bottom sheet with the Figma design https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3481-27050&m=dev.
Made the bottom sheet take as much space as needed and when it takes more than a screenful, the option button list is scrollable.
Screen recording (screenful with top and bottom shadows)
action.sheet.screenful.mp4
Fixes: #90