zulip-flutter icon indicating copy to clipboard operation
zulip-flutter copied to clipboard

action_sheet: Redesign bottom sheet

Open sm-sayedi opened this issue 1 year ago • 7 comments

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)

https://github.com/user-attachments/assets/71880a57-3270-4278-925f-8c79c2f8d3f2

Fixes: #90

sm-sayedi avatar Jul 31 '24 17:07 sm-sayedi

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

Also added a comment in the previous sub-thread.

sm-sayedi avatar Aug 08 '24 19:08 sm-sayedi

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 avatar Aug 08 '24 21:08 sm-sayedi

Thanks @chrisbobbe for the review! Changes pushed. PTAL.

Quoting from #853 comment:

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

Updated one screenshot and added a screen recording of how this looks now.

sm-sayedi avatar Aug 19 '24 12:08 sm-sayedi

Thanks for the comments @gnprice. Pushed the new revision. This time only checked for the "Star message" and "Cancel" buttons to test if the action sheet is shown.

sm-sayedi avatar Aug 22 '24 05:08 sm-sayedi

Thanks @chrisbobbe for the review! Changes pushed.

sm-sayedi avatar Aug 22 '24 22:08 sm-sayedi

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 avatar Sep 30 '24 05:09 sm-sayedi

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

PIG208 avatar Oct 08 '24 00:10 PIG208

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

sm-sayedi avatar Oct 11 '24 15:10 sm-sayedi

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

sm-sayedi avatar Oct 12 '24 04:10 sm-sayedi

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 avatar Oct 17 '24 05:10 gnprice