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

Message List from Same User is now Distinguishable via Surface Tint on Long Press #1152

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Gaurav-Kushwaha-1225
Copy link

Changes Made

This issue resolved by:

  • Wrapping the MessageWithPossibleSender with a Container
  • Then, giving it a color property on a condition that when isMessageActionSheetOpen is true show the surface tint, else remain Colors.transparent.

For this,

  • I shifted the MessageWithPossibleSender from StatelessWidget to StatefulWidget.
  • Added a nullable VoidCallback? onDismiss method in parameters of showMessageActionSheet or BottomSheet for updating the Surface Tint of message.

Fixes #1142

Screenshots

Light Theme Dark Theme
video_2024-12-13_13-16-02.mp4
video_2024-12-13_13-16-08.mp4

@alya
Copy link
Collaborator

alya commented Dec 13, 2024

Please post screenshots of your changes in the PR description, as videos are hard to review.

@Gaurav-Kushwaha-1225
Copy link
Author

Please post screenshots of your changes in the PR description, as videos are hard to review.

Changes

1st

  • shifting the MessageWithPossibleSender widget from StatelessWidget to StatefulWidget.
  • adding isMessageActionSheetOpen and pressedTint as parameters of MessageWithPossibleSender for making surface value tint to transparent after bottom sheet is closed.
  • Screenshot:

2nd

  • Wrapped with Container with color property on isMessageActionSheetOpen condition.
  • Updating the function of onLongPress of MessageWithPossibleSender widget. That, when isMessageActionSheetOpen is true show the surface tint.
  • Screenshot:

3rd

  • Just adding a nullable VoidCallback? onDismiss method in parameters of showMessageActionSheet and _showActionSheet for getting response of closing of bottom sheet.
  • Screenshots:

@chrisbobbe
Copy link
Collaborator

Alya means before/after screenshots of the app's UI. We can read your code easily without screenshots :)

@chrisbobbe
Copy link
Collaborator

Also I see there's a failing check; please run tests locally (tools/check) and fix any failures.

@Gaurav-Kushwaha-1225
Copy link
Author

Alya means before/after screenshots of the app's UI. We can read your code easily without screenshots :)

I am sorry for the confusion.
Here, are the screenshots:

Before After

Also I see there's a failing check; please run tests locally (tools/check) and fix any failures.

I have updated the changes and there are no failing checks and conflicts now.
Thank you.

@alya
Copy link
Collaborator

alya commented Dec 16, 2024

Are those before/after scheenshots the same? The point is to show the differences from your PR's changes.

@Gaurav-Kushwaha-1225
Copy link
Author

Gaurav-Kushwaha-1225 commented Dec 17, 2024

Are those before/after scheenshots the same? The point is to show the differences from your PR's changes.

@alya,
In the screenshots, the message on which the user has long pressed, that message has appeared a surface tint color separating from rest other messages, in both light and dark theme.
This was the issue to resolve i.e. to give a surface tint to those messages where long press event has occurred.

@alya
Copy link
Collaborator

alya commented Dec 17, 2024

Oh, I see it now.

Is it happening in the dark theme screenshot? I don't see a difference, so if the effect is there, it's too subtle.

@alya
Copy link
Collaborator

alya commented Dec 17, 2024

It's also best to take screenshots that are identical other than the change your PR makes, to make them easy to compare.

@Gaurav-Kushwaha-1225
Copy link
Author

Oh, I see it now.

Is it happening in the dark theme screenshot? I don't see a difference, so if the effect is there, it's too subtle.

@alya
Yeah, actually I took the same tint color for both themes. I saw in the figma file both are different for different themes.
I will update the PR in few minutes.
Sorry, for my inconvenience.

@alya
Copy link
Collaborator

alya commented Dec 17, 2024

Sure, please be mindful to review your own work carefully next time. Posting screenshots is one of the things that should help you identify any problems yourself, before asking someone else to take a look.

@Gaurav-Kushwaha-1225
Copy link
Author

Gaurav-Kushwaha-1225 commented Dec 17, 2024

@alya

Here, is the updated PR.

Requirements according to Figma File

Before and After Screenshots

NOTE: In after screenshots, you may see the colors appearing to be a bit different (or less light/dark) from actual figma. This is because the action sheet is opened in my after screenshots which are not in figma file.

Before After

Both pressedTint colors in light and dark theme are according to the Figma File i.e. RGBA(0, 0, 0, 0.04) for light theme and RGBA(1, 1, 1, 0.04) for dark theme.

@PIG208
Copy link
Member

PIG208 commented Dec 17, 2024

Hi @Gaurav-Kushwaha-1225! Thanks for the updates. For the PR to be reviewable, we need tests for this: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

Please also pay attention to the point on commit style guide from that link, and organize the commit history coherently.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Dec 23, 2024
@chrisbobbe
Copy link
Collaborator

I see you're adding merge commits; Zulip doesn't use those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

msglist: Message bounds hard to see in a run of messages from the same user
5 participants