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.

Sign up for GitHub

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

Jump to bottom

msglist: Use distinct background colors for DM messages. #1305

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

msglist: Use distinct background colors for DM messages. #1305

wants to merge 2 commits into from
+31 −10

Conversation

E-m-i-n-e-n-c-e
Copy link

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e commented Jan 24, 2025 •
edited
Loading

This PR modifies DmMessage and its DateSeparator to use a different background color from StreamMessage. This matches the web app's behavior where DM messages have a distinct background color.

Added new theme colors to MessageListTheme(These colors match the web app version)

  • Light theme: hsl(45deg 20% 96%)

  • Dark theme: hsl(46deg 7% 16%)

Fixes #681.

Edit: Modified smoke test for light/dark/lerped to test both message types. It now creates two example messages, one DM and one StreamMessage, and checks their background colors.

Screenshots:

Light mode Dark mode
Image Image
Image Image

chrisbobbe
chrisbobbe reviewed Jan 24, 2025
View reviewed changes
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.

Also, the commit message seems longer than it needs to be; it repeats details that are already clear from the Fixes #681 line and the changes made. Also the paragraphs should be wrapped as specified in Zulip's commit style guide.

lib/widgets/message_list.dart Outdated Show resolved Hide resolved
lib/widgets/message_list.dart Outdated Show resolved Hide resolved
lib/widgets/message_list.dart Outdated Show resolved Hide resolved
lib/widgets/message_list.dart Outdated Show resolved Hide resolved
lib/widgets/message_list.dart Outdated Show resolved Hide resolved
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the DmBackground branch 3 times, most recently from f2707ac to 112baba Compare January 25, 2025 09:45
chrisbobbe
chrisbobbe reviewed Jan 28, 2025
View reviewed changes
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! LGTM except for one new idea I had below.

lib/widgets/message_list.dart Outdated Show resolved Hide resolved
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the DmBackground branch 2 times, most recently from 104d569 to 852c509 Compare January 29, 2025 13:08
@PIG208 PIG208 mentioned this pull request Jan 29, 2025
Open
@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e force-pushed the DmBackground branch 3 times, most recently from 236ea51 to c3d37a9 Compare January 30, 2025 09:46
@E-m-i-n-e-n-c-e
Copy link
Author

Thanks! LGTM except for one new idea I had below.

Made the changes. Do tell me if there is anything else

@PIG208 PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 30, 2025
chrisbobbe
chrisbobbe reviewed Feb 4, 2025
View reviewed changes
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. Also, this needs tests that check the message and date-separator background colors for the two types of messages.

lib/widgets/message_list.dart Outdated Show resolved Hide resolved
@gnprice gnprice added this to the M6: Post-launch milestone Feb 6, 2025
@gnprice
Copy link
Member

gnprice commented Feb 6, 2025

Thanks @E-m-i-n-e-n-c-e for your work so far on this, and @chrisbobbe for the reviews!

Given that this issue is for the M6 Post-launch milestone, and that this PR doesn't yet have tests so still has significant work ahead before we can merge it, let's put it on hold for now until the launch is ready — see announcement.

The bug fix in that second commit would be good to include sooner, though. @E-m-i-n-e-n-c-e would you send a quick PR with just that change? (It's fine to leave that without a test — that line is in highly mechanical code which we should ultimately assure the correctness of by generating it, not testing it.) Then we can get that merged.

@gnprice gnprice marked this pull request as draft February 6, 2025 02:00
@E-m-i-n-e-n-c-e
msglist: Fix DM header color lerp bug.
fa4b9f3 
Fixed a bug in MessageListTheme.lerp() where dmRecipientHeaderBg was using
streamMessageBgDefault instead of dmRecipientHeaderBg for interpolation.
@E-m-i-n-e-n-c-e
Copy link
Author

Thanks @E-m-i-n-e-n-c-e for your work so far on this, and @chrisbobbe for the reviews!

Given that this issue is for the M6 Post-launch milestone, and that this PR doesn't yet have tests so still has significant work ahead before we can merge it, let's put it on hold for now until the launch is ready — see announcement.

The bug fix in that second commit would be good to include sooner, though. @E-m-i-n-e-n-c-e would you send a quick PR with just that change? (It's fine to leave that without a test — that line is in highly mechanical code which we should ultimately assure the correctness of by generating it, not testing it.) Then we can get that merged.

I have reordered the commits. Will add the tests in the next push

@E-m-i-n-e-n-c-e
msglist: Use distinct background colors for DM messages.
31151be 
This commit modifies DmMessage and its DateSeparator to use a
different background color from StreamMessage.

Fixes zulip#681.
@E-m-i-n-e-n-c-e
Copy link
Author

I have added the tests as well. Please take another look at the changes

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e marked this pull request as ready for review February 6, 2025 06:03
@chrisbobbe
Copy link
Collaborator

The bug fix in that second commit would be good to include sooner, though. @E-m-i-n-e-n-c-e would you send a quick PR with just that change? (It's fine to leave that without a test — that line is in highly mechanical code which we should ultimately assure the correctness of by generating it, not testing it.) Then we can get that merged.

After you send that separate PR, please link to it here so it's easy to find.

@E-m-i-n-e-n-c-e
Copy link
Author

The bug fix in that second commit would be good to include sooner, though. @E-m-i-n-e-n-c-e would you send a quick PR with just that change? (It's fine to leave that without a test — that line is in highly mechanical code which we should ultimately assure the correctness of by generating it, not testing it.) Then we can get that merged.

After you send that separate PR, please link to it here so it's easy to find.

#1336

@E-m-i-n-e-n-c-e E-m-i-n-e-n-c-e mentioned this pull request Feb 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnprice gnprice gnprice left review comments

@chrisbobbe chrisbobbe chrisbobbe left review comments

Assignees
No one assigned
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Milestone
M6: Post-launch
Development

Successfully merging this pull request may close these issues.

msglist: Use special background color for DM messages
4 participants
@E-m-i-n-e-n-c-e @gnprice @chrisbobbe @PIG208