Skip to content
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

action_sheet: Redesign bottom sheet #853

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

sm-sayedi
Copy link
Collaborator

@sm-sayedi sm-sayedi commented Jul 31, 2024

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.

Before After
After (more than a screenfull) After - Dark

Screen recording (screenful with top and bottom shadows)

action.sheet.screenful.mp4

Fixes: #90

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch 2 times, most recently from 8387fb7 to 3444df5 Compare August 2, 2024 02:37
Comment on lines 39 to 62
showDraggableScrollableModalBottomSheet<void>(
final designVariables = DesignVariables.of(context);
showModalBottomSheet<void>(
Copy link
Collaborator Author

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.

Copy link
Member

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 in showDraggableScrollableModalBottomSheet as there is showDragHandle property available in showModalBottomSheet.

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.

Copy link
Collaborator

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
image image

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

https://github.com/flutter/flutter/blob/ba31bf8c0/packages/flutter/lib/src/material/bottom_sheet.dart#L402

    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.

Copy link
Collaborator

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.)

@sm-sayedi sm-sayedi marked this pull request as ready for review August 2, 2024 02:48
@sm-sayedi sm-sayedi added the buddy review GSoC buddy review needed. label Aug 2, 2024
@sm-sayedi sm-sayedi requested a review from Khader-1 August 2, 2024 02:49
@sm-sayedi sm-sayedi modified the milestone: Beta 3: Summer 2024 Aug 3, 2024
@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch from 3444df5 to 664fce5 Compare August 4, 2024 04:26
Copy link
Collaborator

@rajveermalviya rajveermalviya left a 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.

Comment on lines 102 to 105
style: MenuItemButton.styleFrom(
backgroundColor: designVariables.actionSheetMenuButtonBackground,
foregroundColor: designVariables.actionSheetMenuButtonForeground,
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit indentation:

Suggested change
style: MenuItemButton.styleFrom(
backgroundColor: designVariables.actionSheetMenuButtonBackground,
foregroundColor: designVariables.actionSheetMenuButtonForeground,
),
style: MenuItemButton.styleFrom(
backgroundColor: designVariables.actionSheetMenuButtonBackground,
foregroundColor: designVariables.actionSheetMenuButtonForeground,
),

Comment on lines 102 to 103
style: MenuItemButton.styleFrom(
backgroundColor: designVariables.actionSheetMenuButtonBackground,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-08-06 at 20 15 19

Let's override default padding and specify it explicitly, the suggested values here are adapted from Figma design:

Suggested change
style: MenuItemButton.styleFrom(
backgroundColor: designVariables.actionSheetMenuButtonBackground,
style: MenuItemButton.styleFrom(
padding: const EdgeInsets.fromLTRB(16, 12, 16, 12),
backgroundColor: designVariables.actionSheetMenuButtonBackground,

onPressed: () => onPressed(context),
child: Text(label(zulipLocalizations)));
child: Text(label(zulipLocalizations), style: const TextStyle(fontSize: 16)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-08-06 at 20 23 14

Figma design specifies different fontSize and fontWeight:

Suggested change
child: Text(label(zulipLocalizations), style: const TextStyle(fontSize: 16)));
child: Text(label(zulipLocalizations),
style: const TextStyle(fontSize: 20)
.merge(weightVariableTextStyle(context, wght: 600))));

),
onPressed: () => Navigator.pop(context),
child: Text(ZulipLocalizations.of(context).dialogCancel,
style: const TextStyle(fontSize: 16)),
Copy link
Collaborator

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.

Comment on lines 443 to 494
return ElevatedButton(
style: ElevatedButton.styleFrom(
elevation: 0,
backgroundColor: designVariables.actionSheetCancelButtonBackground,
foregroundColor: designVariables.actionSheetCancelButtonForeground,
shadowColor: Colors.transparent,
shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(7)),
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use TextButton here:

Suggested change
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)),
),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also add explicit padding of EdgeInsets.all(10) as per Figma design:

Screenshot 2024-08-06 at 21 50 27

),
),
),
const MessageActionSheetCancelButton(),
Copy link
Collaborator

@rajveermalviya rajveermalviya Aug 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There needs to be 8px gap between Menu buttons and Cancel button as per design:

Screenshot 2024-08-06 at 21 52 34

Comment on lines 60 to 81
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),
Copy link
Collaborator

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(),

Copy link
Collaborator Author

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. 🙂

message: message,
messageListContext: context,
),
CopyMessageTextButton(message: message, messageListContext: context),
Copy link
Collaborator

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".

),
CopyMessageTextButton(message: message, messageListContext: context),
CopyMessageLinkButton(message: message, messageListContext: context),
ShareButton(message: message, messageListContext: context),
Copy link
Collaborator

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"

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch 2 times, most recently from e798189 to 50e8b68 Compare August 8, 2024 19:25
@sm-sayedi
Copy link
Collaborator Author

sm-sayedi commented Aug 8, 2024

Thanks @rajveermalviya for the detailed review! It was really helpful. Revision pushed. PTAL.

Also added a comment in the previous sub-thread.

Copy link
Collaborator

@rajveermalviya rajveermalviya left a 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.

onPressed: () => onPressed(context),
child: Text(label(zulipLocalizations)));
child: Text(label(zulipLocalizations),
style: const TextStyle(fontSize: 20, fontWeight: FontWeight.w600),
Copy link
Collaborator

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:

Suggested change
style: const TextStyle(fontSize: 20, fontWeight: FontWeight.w600),
style: const TextStyle(fontSize: 20)
.merge(weightVariableTextStyle(context, wght: 600)),

Copy link
Collaborator

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.

@rajveermalviya rajveermalviya added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Aug 8, 2024
Comment on lines 71 to 74
// 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()),
Copy link
Collaborator Author

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.

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch from 50e8b68 to f28a083 Compare August 8, 2024 21:05
@sm-sayedi
Copy link
Collaborator Author

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.

@sm-sayedi sm-sayedi added the maintainer review PR ready for review by Zulip maintainers label Aug 8, 2024
@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch 5 times, most recently from 505931d to f19b211 Compare August 15, 2024 04:13
@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch from 9c07846 to ccff5fb Compare August 22, 2024 22:24
@sm-sayedi
Copy link
Collaborator Author

Thanks @chrisbobbe for the review! Changes pushed.

Copy link
Collaborator

@chrisbobbe chrisbobbe left a 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.

Comment on lines 489 to 614
tapTargetSize: MaterialTapTargetSize.shrinkWrap,
minimumSize: Size.zero,
Copy link
Collaborator

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)

scrollController: scrollController,
builder: (_, scrollController) => SizedBox(
height: math.max(
16 - scrollController.position.extentBefore,
Copy link
Collaborator

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).

@sm-sayedi
Copy link
Collaborator Author

Thanks @chrisbobbe for the review and the proposed approach! I prefer yours and there is no need for the "dynamic padding".
Changes pushed. Please have a look!

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch 2 times, most recently from aa286c7 to ab74659 Compare October 7, 2024 02:23
Copy link
Collaborator

@chrisbobbe chrisbobbe left a 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.

group('MessageActionSheetCancelButton', () {
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;

Finder cancelButtonFinder() => find.text(zulipLocalizations.dialogCancel);
Copy link
Collaborator

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?

Copy link
Member

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

Copy link
Collaborator Author

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?

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. 🙂

Copy link
Member

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);

Comment on lines 109 to 116
],
),
),
const MessageActionSheetCancelButton(),
],
),
),
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indentation

@chrisbobbe chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed mentor review GSoC mentor review needed. maintainer review PR ready for review by Zulip maintainers labels Oct 7, 2024
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Oct 7, 2024
@PIG208
Copy link
Member

PIG208 commented Oct 8, 2024

_InsetShadowBox was previously implemented with a SingleChildScrollView wrapped by a NotificationListener.

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 InsetShadowBox available separately.

Copy link
Member

@gnprice gnprice left a 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.

Comment on lines -91 to -94
// 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,
Copy link
Member

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.

@@ -431,3 +505,28 @@ class ShareButton extends MessageActionSheetMenuItemButton {
}
}
}

class MessageActionSheetCancelButton extends StatelessWidget {
Copy link
Member

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.

group('MessageActionSheetCancelButton', () {
final zulipLocalizations = GlobalLocalizations.zulipLocalizations;

Finder cancelButtonFinder() => find.text(zulipLocalizations.dialogCancel);
Copy link
Member

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

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch from ab74659 to 92a6ae8 Compare October 11, 2024 15:30
@sm-sayedi
Copy link
Collaborator Author

Thanks @chrisbobbe and @gnprice for the reviews. Changes pushed.

Copy link
Member

@gnprice gnprice left a 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.)

Comment on lines 85 to 86
child: Column(spacing: 1, children: optionButtons),
)))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
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

@sm-sayedi sm-sayedi force-pushed the issue-90-bottom-sheet-redesign branch from 92a6ae8 to 84f8eb6 Compare October 12, 2024 02:47
@sm-sayedi
Copy link
Collaborator Author

Thanks for the review @gnprice. Changes applied, PTAL.

@gnprice
Copy link
Member

gnprice commented Oct 17, 2024

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!

@gnprice gnprice force-pushed the issue-90-bottom-sheet-redesign branch from 84f8eb6 to f8ddff2 Compare October 17, 2024 05:09
@gnprice gnprice merged commit f8ddff2 into zulip:main Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bottom sheet redesign
5 participants