# ui: Changes the design of Bottom Action sheet. #585
Conversation
|
I think it would be great if you would include screen recordings for your UI changes. It will make it easier for code reviewers to see your changes and get your PR reviewed faster. |
|
You must also add tests for the changes you have added and make sure that the existing tests are passing. |
Ok Sure I'll add a screen recording for PR. |
|
@gnprice Can you please review this PR? |
There was a problem hiding this comment.
Before we can review this PR in detail, it will need tests as mentioned above. See the README at "Submitting a pull request":
https://github.com/zulip/zulip-flutter#submitting-a-pull-request
See also the Zulip project's Git style guide (linked from that same README section). Instead of one commit that causes the test suite to fail, and a second commit fixing the error, the fix should be squashed in so that the tests pass after each individual commit.
For those reasons I've been assuming you're still working on this PR as a draft, which is why I haven't reviewed it.
I took a quick look just now, though. Here's one piece of high-level feedback on the overall approach.
lib/widgets/action_sheet.dart
Outdated
| late final bool _isFirst; | ||
| late final bool _isLast; | ||
|
|
||
| void setIsFirst(){ | ||
| _isFirst = true; | ||
| } |
There was a problem hiding this comment.
This mutation of a widget object is awfully unidiomatic for Flutter. We'll want to find a solution which doesn't involve mutating any widgets after constructing them.
It's possible that a good solution here will involve making more widgets, or different widgets — the MessageActionSheetMenuItemButton class as a subclass of StatelessWidget is something we made up, and we're free to change it if that helps us structure things well. If you're trying ideas for this and you have questions about whether a particular refactor seems like a good direction, #mobile-team on chat.zulip.org will be a good place to ask.
There was a problem hiding this comment.
Ok Sure, I will try to find some way around to avoid that behaviour. Let's discuss more about that on #mobile-team on chat.zulip.org
Thanks for your review here. |
action_sheet.dartfile there are 3 major changes :showMessageActionSheetmethod to create a list of widgets for the widgets to rendered in the action sheet instead of directly passing it as the children of Column. This also reduced the explicit specification of the rounded border of the first and the last element.MessageActionSheetMenuItemButtonto have the colors and the variables to specify that an element is first or last element in the Column.MessageActionSheetCancelButtonfor the cancel button.first_pr_vid.mp4