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

Tweak dark theme #1213

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

Conversation

Gaurav-Kushwaha-1225
Copy link

@Gaurav-Kushwaha-1225 Gaurav-Kushwaha-1225 commented Dec 27, 2024

About Changes

This PR updates the color variables in DesignVariables and MessageListTheme for a slightly higher contrast in dark theme according to the values in the figma file.

Fixes: #973

Screenshots

Before After

@alya
Copy link
Collaborator

alya commented Dec 27, 2024

Please clean up your commit history and post again to request a review. See here for guidelines.

@Gaurav-Kushwaha-1225
Copy link
Author

Please clean up your commit history and post again to request a review. See here for guidelines.

I’ve cleaned up the commit history by removing unnecessary merge commits into the TweakDarkTheme branch and combining related changes into a single commit for clarity.

This is my first time using rebase -i. During the process, I used pick, squash, and drop commands for various commits. I initially thought there would be separate commit messages for each, but later realized they were combined into one during the rebase.
Although, it asked to enter three commit messages after rebasing, Idk why it happened. Could you please explain why all commit messages were merged into one?
I’d like to understand this better for future contributions.

Apologies for any confusion.

Please review the updated PR.

@gnprice
Copy link
Member

gnprice commented Dec 27, 2024

The #git help channel on chat.zulip.org is a good place to ask questions about using Git.

@Gaurav-Kushwaha-1225
Copy link
Author

Please clean up your commit history and post again to request a review. See here for guidelines.

Hi @alya, please review the updated PR.

@alya
Copy link
Collaborator

alya commented Jan 6, 2025

Thanks! I'll let other folks take a look at this one.

@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Jan 7, 2025
@PIG208
Copy link
Member

PIG208 commented Jan 8, 2025

Hi! Please read through the commit style guide (also previously linked by Alya) linked in our README and fix the commit message: https://github.com/zulip/zulip-flutter?tab=readme-ov-file#submitting-a-pull-request

Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I noted the differences between this revision and the Figma design after going through lib/widgets/message_list.dart and some part of lib/widgets/compose_box.dart for colors used in the widgets.

Some of the comments are just about different colors being used, and others request the addition/removal of variables. It should be fine to include color adjustments all in a single commit, but for adding/removing variables, let's try to have separate commits for the ones added, so it is easier to see which variables they replace. Consider using the NFC tag if a commit is a pure refactor.

I think there are also changes to the autocomplete window, but that should be addressed in #995,

lib/widgets/message_list.dart Outdated Show resolved Hide resolved
lib/widgets/theme.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/theme.dart Outdated Show resolved Hide resolved
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 12, 2025
Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in `MessageListTheme`
as asked.
`foreground`: zulip#1213 (comment)
`dateSeparator`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 12, 2025
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 12, 2025
Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in
`MessageListTheme` as asked.

`foreground`: zulip#1213 (comment)
`dateSeparator`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 12, 2025
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in
`MessageListTheme` as asked.

`foreground`: zulip#1213 (comment)
`dateSeparator`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in `MessageListTheme`
as asked.
`foreground`: zulip#1213 (comment)
`dateSeparator`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Removed `colorMessageHeaderIconInteractive` from
`DesignVariables` and used `title` in place of it
as per the design guidelines.
discussion: zulip#1213 (comment)
Gaurav-Kushwaha-1225 added a commit to Gaurav-Kushwaha-1225/zulip-flutter that referenced this pull request Jan 15, 2025
Removed `senderName` & `recipientHeaderText` from
`MessageListTheme` and used `title` in place of it
as per the design guidelines.
discussion: zulip#1213 (comment)
chrisbobbe and others added 2 commits January 17, 2025 13:07
Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in `MessageListTheme`
as asked.
`foreground`: zulip#1213 (comment)
`dateSeparator`: zulip#1213 (comment)
Added `labelTime` variable in `MessageListTheme`
that replaces the `dateSeparatorText` and
`messageTimestamp` variable according to the latest
design.

`labelTime`: zulip#1213 (comment)
Removed `colorMessageHeaderIconInteractive` from
`DesignVariables` and used `title` in place of it
as per the design guidelines.
discussion: zulip#1213 (comment)
Removed `senderName` & `recipientHeaderText` from
`MessageListTheme` and used `title` in place of it
as per the design guidelines.
discussion: zulip#1213 (comment)
@Gaurav-Kushwaha-1225
Copy link
Author

Hi @PIG208,
I have updated the commits as per your request.

  • Made separate commits as required
  • Removed colorMessageHeaderIconInteractive, senderName & recipientHeaderText
  • Added labelTime in MessageListTheme

Have a review of these changes and tell if anything else is required regarding this issue.

@PIG208 PIG208 self-requested a review January 21, 2025 16:17
Copy link
Member

@PIG208 PIG208 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update! The changes mostly look good to me, except for the first commit that adjusts a bunch of colors. It turned out that "[nfc]" is not applicable for the commits, and that's fine. They are useful to mark commits that are meant to incur no behavioral changes, so the reviewer can focus on the non-nfc commits.

The final commit of the PR needs a line to fix the issue:

...(other stuff)...

Fixes: #973

lib/widgets/theme.dart Show resolved Hide resolved
@@ -55,14 +55,14 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme: Adjust foreground & dateSeparator colors

Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in `MessageListTheme`
as asked.

This commit is adjusting more colors than the commit message is suggesting. Some of them is probably irrelevant because we refactored them away later, right? It will be cleaner to make this the final commit in the sequence.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I edit the commit message too after reordering the commits, like the below one;

theme: Adjust foreground & dateSeparator colors

Adjusted the colors for `foreground` in
`DesignVariables` and `dateSeparator` in `MessageListTheme`
as asked. 
Updated `dateSeparator` and `dmRecipientHeaderBg` values acc. to   
their values in figma file.
`foreground`: https://github.com/zulip/zulip-flutter/pull/1213#discussion_r1908368175
`dateSeparator`: https://github.com/zulip/zulip-flutter/pull/1213#discussion_r1908328416

lib/widgets/theme.dart Show resolved Hide resolved
lib/widgets/theme.dart Show resolved Hide resolved
recipientHeaderText: const HSLColor.fromAHSL(0.8, 0, 0, 1).toColor(),
dateSeparator: Colors.white.withValues(alpha: 51),
dateSeparatorText: const HSLColor.fromAHSL(0.5, 0, 0, 1).toColor(),
dmRecipientHeaderBg: const HSLColor.fromAHSL(1, 0, 0, 0.14).toColor(),
Copy link
Member

@PIG208 PIG208 Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you identify where this color (dmRecipientHeaderBg) comes from? I looked at https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3702-28206&p=f&t=dBCq6jmgNML8Joly-0 but couldn't find it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • This dmRecipientHeaderBg is the light-colored top bar in below screen.
  • Though, it is represented using bg-top-bar in the figma design file with value HSLColor.fromAHSL(1, 0, 0, 0.14).toColor().

lib/widgets/message_list.dart Show resolved Hide resolved
lib/widgets/message_list.dart Show resolved Hide resolved
lib/widgets/message_list.dart Show resolved Hide resolved
lib/widgets/theme.dart Outdated Show resolved Hide resolved
@@ -32,9 +32,7 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
dateSeparator: Colors.black,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theme[nfc]: Remove senderName & recipientHeaderText 

commit message nit: similarly, mentioning title in the commit summary is helpful

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't get why you are saying to add title in this commit message.

theme: Replace `dateSeparatorText` and `messageTimestamp` with `labelTime`

This commit message as you have mentioned above is fine for this commit.
Please correct me if I didn't understand properly at any place.

lib/widgets/message_list.dart Show resolved Hide resolved
@Gaurav-Kushwaha-1225
Copy link
Author

Thanks @PIG208 for the review.
I will look after the requested changes above and update this PR soon.

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.

Tweak dark theme for slightly higher contrast
6 participants