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

New design for PM and stream recipient bars. #23782

Merged
merged 12 commits into from Apr 11, 2023

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented Dec 7, 2022

This PR is aimed towards making significant progress in the direction of #22021.
Figma design was a great source used for finalizing some of the details on space between elements in this PR.
The changes that are mentioned in the issue but are not included in the PR are:

  • The recipient bar interactive icons like message like, edit pencil etc.
  • I didn't change the font size of text to match the new design, but did increase the height of the recipient bar.

Screenshot 2022-12-08 at 12 39 18 AM

@amanagr amanagr changed the title New desing for PM and stream recipient bars. New design for PM and stream recipient bars. Dec 7, 2022
@amanagr
Copy link
Member Author

amanagr commented Dec 7, 2022

Ignore the CI failures for now. Once the design is finalized, I can easily take care of it now.

@alya
Copy link
Contributor

alya commented Dec 7, 2022

It's cool being able to play around with this! Here are the issues I spotted...

@alya
Copy link
Contributor

alya commented Dec 7, 2022

  1. The lines on the sides curve in after each message, rather than going straight down.

Screen Shot 2022-12-07 at 12 18 42 PM

@alya
Copy link
Contributor

alya commented Dec 7, 2022

  1. I noticed that this removes the stream color sidebar. While that's definitely the goal, integrating that change is currently blocked on #9910, so I guess I don't know if it's best to do that here or leave it as a follow-up.

@alya
Copy link
Contributor

alya commented Dec 7, 2022

I noticed some awkward behaviors at the top of the message feed:

  1. For some reason when there are two header bars at the very top, they aren't quite aligned horizontally.

Screen Shot 2022-12-07 at 12 20 01 PM

  1. In the same screenshot, there's an extra vertical line in the top right. You can also see this with just one header bar:

Screen Shot 2022-12-07 at 12 22 10 PM

  1. With some playing around with scrolling, I got into a state where the header bar is cut off in an awkward way:

Screen Shot 2022-12-07 at 12 20 19 PM

  1. It is possible to get a significant chunk of a message above the top header bar, which I don't think you can do on main, and it seems undesirable.

Screen Shot 2022-12-07 at 12 21 09 PM

@alya
Copy link
Contributor

alya commented Dec 7, 2022

  1. The globe vertical alignment definitely looks too low to me; not sure about the other icons.
    Screen Shot 2022-12-07 at 12 34 06 PM

@amanagr
Copy link
Member Author

amanagr commented Dec 8, 2022

The icons in your screenshot look different from mine for some reason.
Screenshot 2022-12-08 at 5 52 12 AM

@amanagr
Copy link
Member Author

amanagr commented Dec 8, 2022

Updated the PR to fix the 1st bug you noticed.

@alya
Copy link
Contributor

alya commented Dec 8, 2022

The icons in your screenshot look different from mine for some reason.

Huh, maybe a zoom issue?

@terpimost
Copy link
Collaborator

Good progress. I see the horizontal spacing is a bit different. It just feels that there is too much space for the icon:
image
Comparing with a correct icon in PM I think we should move icon 1px left and the text 2 px left:
image

For some reason switching the theme makes headers stuck in the previous theme. But reloading the page resolves that:
image (could be some glitch on my end or some race condition)

About the background color of the PMs. Let's reevaluate that once we can see messages on the proper dark background instead of this bright blue.

@alya I highly recommend to have dark theme background change together (close) to merging that

@terpimost
Copy link
Collaborator

terpimost commented Dec 10, 2022

I'm not sure the color collation is correct for some cases. For example if I take hashtag icon color from this (# 9cbcda):
image
and apply it in my example:
image
the calculated bg color for the bar is # 1f262c, while on this PR calculation is # 21282e

@amanagr
Copy link
Member Author

amanagr commented Dec 14, 2022

@terpimost addressed all your concerns. Please review again!

@amanagr
Copy link
Member Author

amanagr commented Feb 15, 2023

wow, didn't realize this is difficult to rebase now.

@terpimost
Copy link
Collaborator

I can't review it due to error
image

@amanagr
Copy link
Member Author

amanagr commented Feb 16, 2023

Yeah, it is not ready for review due to rebase conflicts, I am working on it right now, I have to basically rewrite the whole PR.

@amanagr
Copy link
Member Author

amanagr commented Feb 16, 2023

@terpimost this should be ready for another review now.

@terpimost
Copy link
Collaborator

I still see some problems
image
image

What should I do?

stream_privacy template has conditions to show stream privacy icon
for all the cases, so we use it here since we also want to show
`#` icon for public streams.
@@ -78,6 +80,7 @@ export function switch_to_dark_theme() {
command: "/night",
on_success(data) {
dark_theme.enable();
message_lists.update_recipient_bar_background_color();
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

These slash commands shouldn't be doing UI updates at all, should they? I would expect the server_events system to be responsible for them.

Copy link
Member Author

Choose a reason for hiding this comment

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

My guess is we have dark_theme.enable(); here so that user feels that the color change to dark theme was quick. Since, dark_theme.enable(); is here, I need to do message_lists.update_recipient_bar_background_color(); here as well.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

OK; I suppose we can add a comment. In any case, we could look at making dark_theme.enable call message_lists.update_recipient_bar_background_color as a follow-up I suppose.

web/styles/zulip.css Outdated Show resolved Hide resolved
Following important changes are being made here:
* color_class lib is removed since it not used anywhere now.
* We don't need the `dark_background` class since the background
  color is already adjusted based on color scheme. So, all
  instances of it being used is removed.
This mostly includes the CSS changes for recipient row design,
most of the HTML and JS changes are present in previous commits to
this.
This commit doesn't modify the lock and hashtag icon in settings.
@amanagr
Copy link
Member Author

amanagr commented Apr 11, 2023

Screenshot 2023-04-11 at 11 39 01 AM

I made an additional change:

Screenshot 2023-04-11 at 11 41 19 AM

since I reverted it as part of Evy's PR I think, but it fixes the alignment now for date and time.

image

@timabbott
Copy link
Sponsor Member

This looks great, tagging to merge once CI passes; I'll deploy main including this to chat.zulip.org tomorrow. Thanks for all the work on this @amanagr and @evykassirer!

(#23782 (comment) seems pretty minor as a code cleanup/comment tweak, so let's leave that for a follow-up -- given it's not a functional change and is mostly a preexisting detail I noticed, there's no reason to block on it.)

@timabbott timabbott enabled auto-merge (rebase) April 11, 2023 06:21
@timabbott
Copy link
Sponsor Member

Do we need an issue for updating the stream privacy icon (i.e. the last commit work) for the settings pages? Could make sense for @sahil839 to take over that portion of it.

@amanagr
Copy link
Member Author

amanagr commented Apr 11, 2023

I will just open a PR for it, it seems like a simple delete.

@timabbott timabbott merged commit 8c74475 into zulip:main Apr 11, 2023
13 checks passed
@amanagr amanagr deleted the msg_recipient_redesign branch April 11, 2023 06:45
andersk added a commit to andersk/zulip that referenced this pull request Aug 31, 2023
It’s unused since commit 7e47300
(zulip#23782).

Signed-off-by: Anders Kaseorg <anders@zulip.com>
timabbott pushed a commit that referenced this pull request Aug 31, 2023
It’s unused since commit 7e47300
(#23782).

Signed-off-by: Anders Kaseorg <anders@zulip.com>
Vector73 pushed a commit to Vector73/zulip that referenced this pull request Sep 7, 2023
It’s unused since commit 7e47300
(zulip#23782).

Signed-off-by: Anders Kaseorg <anders@zulip.com>
freshpex pushed a commit to freshpex/zulip that referenced this pull request Sep 14, 2023
It’s unused since commit 7e47300
(zulip#23782).

Signed-off-by: Anders Kaseorg <anders@zulip.com>
freshpex pushed a commit to freshpex/zulip that referenced this pull request Sep 14, 2023
It’s unused since commit 7e47300
(zulip#23782).

Signed-off-by: Anders Kaseorg <anders@zulip.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants