Skip to content

ui: Changes the design of Bottom Action sheet. #623

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

Closed
wants to merge 1 commit into from

Conversation

Sahilpervez
Copy link

  • Closes Bottom sheet redesign #90
  • This PR consists the modification of design of action sheet that pops up when any message is long pressed.
  • In action_sheet.dart file there are 3 major changes :
    • Changed the abstract class MessageActionSheetMenuItemButton to have the colors and a variable final int isRounded to specify that an element is first or last element .
      • If isRounded == 1 then it is the first element of the list
      • If isRounded == 2 then it is the last element of the list
      • And by default isRounded == 0 which signifies that the element is neither first or last element.
    • Changed the design of the bottom sheet according to the given figma design.
    • Added a widget MessageActionSheetCancelButton for the cancel button.
first_pr_vid.mp4

@abelaba
Copy link
Contributor

abelaba commented Apr 19, 2024

Hey @Sahilpervez. The UI looks great, but before the code maintainers review your code you should make sure that you write tests for your changes. https://github.com/zulip/zulip-flutter?tab=readme-ov-file#writing-tests.

You should also add a description on your commit message
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-messages

@Sahilpervez
Copy link
Author

Hey @Sahilpervez. The UI looks great, but before the code maintainers review your code you should make sure that you write tests for your changes. https://github.com/zulip/zulip-flutter?tab=readme-ov-file#writing-tests.

You should also add a description on your commit message https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html#commit-messages

Hey @abelaba thanks for your guidance, I'll make sure to write the tests for the changes and squash the commits then open the PR.

@gnprice
Copy link
Member

gnprice commented Apr 26, 2024

In addition to @abelaba's observation that this will need tests, two quick other bits of high-level feedback:

a variable final int isRounded to specify that an element is first or last element .

  • If isRounded == 1 then it is the first element of the list
  • If isRounded == 2 then it is the last element of the list
  • And by default isRounded == 0 which signifies that the element is neither first or last element.

First, instead of an int with magic values 0, 1, 2, this would be much cleaner as a Dart enum.

Second, instead of a video screen recording it'd be better to show static screenshots of key situations. Those are generally much more helpful for review. Generally a video should be used only when there's something the PR is changing that can't be captured in screenshots (but can be in a video), which I don't think there is in this PR.

@gnprice
Copy link
Member

gnprice commented May 14, 2024

Closing this PR for now in order to clear it from our review queue. @Sahilpervez if at some point you'd like to pick this PR back up, please feel free to re-open it after addressing the feedback above.

@gnprice gnprice closed this May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bottom sheet redesign
3 participants