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

UI redesign: Recipient header bar (topic title bar) #22430

Closed
wants to merge 1 commit into from

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented Jul 9, 2022

Issue: #22021

Discussion: https://chat.zulip.org/#narrow/stream/431-redesign-project/topic/recipient.20bar.20.2322021

@amanagr amanagr marked this pull request as draft July 9, 2022 09:29
@amanagr amanagr force-pushed the recipient_bar_redesign branch 3 times, most recently from d8d8555 to 5ca4bcb Compare July 13, 2022 09:34
@amanagr
Copy link
Member Author

amanagr commented Jul 13, 2022

image

image

@amanagr
Copy link
Member Author

amanagr commented Jul 13, 2022

We should increase the color contrast between the message and the background in the night theme. Right now, they look very alike.

@alya
Copy link
Contributor

alya commented Jul 15, 2022

Thanks for working on this -- it's cool to see!

Could you clarify why the TODO items in the description for this PR are relevant to #22021? E.g. I wouldn't expect the recipient header bar change to touch popovers, the compose box, etc. but maybe I'm missing something.

@alya
Copy link
Contributor

alya commented Jul 15, 2022

The PR also seems to make some changes that are separate from what was described in this issue. E.g. is it solving parts of #21750 as well? (I believe @sayamsamal was planning to pick that one up?)

@alya
Copy link
Contributor

alya commented Jul 15, 2022

Commenting on just the recipient header bar aspect of it, it feels to me like the contrast between # and the background color is generally too low in the current version. I think we likely want the background color to be lighter, like in @terpimost 's original design.

Screen Shot 2022-07-14 at 10 05 57 PM
Screen Shot 2022-07-14 at 10 05 48 PM

(If that part wasn't ready to review yet, I'm happy to check again later. :))

@amanagr
Copy link
Member Author

amanagr commented Jul 15, 2022

I wouldn't expect the recipient header bar change to touch popovers, the compose box, etc. but maybe I'm missing something.

It is due to the change in background color of the app. The new background color doesn't look good with the current colors of these elements.

@alya
Copy link
Contributor

alya commented Jul 15, 2022

Hmm, OK, but updating the background color of the app was not part of #22021 either...

@terpimost
Copy link
Collaborator

@amanagr I think the problem that you don't add the proper background color for the bar and for the light theme the background color is off. Here are clarifications https://www.loom.com/share/d531f451452e44e089bea0a6f9d6e496

@amanagr
Copy link
Member Author

amanagr commented Jul 20, 2022

@amanagr I think the problem that you don't add the proper background color for the bar and for the light theme the background color is off. Here are clarifications https://www.loom.com/share/d531f451452e44e089bea0a6f9d6e496

Thanks for the great review video. I was using a plugin to convert hex values into HSL values, and I guess it wasn't working correctly.

@terpimost
Copy link
Collaborator

@amanagr for some reason I see the color is 3% now
image
but in the original design it was 5%? and later we were talking about making it bigger.

Now as I see that in action I vote for having 8% for both dark and light themes. And we might make it 10% once more people will try that in action.

P.S. Could you try to set up colors via HSLA spelling? or RGBA ? instead of hex...
I switched it in dev tools just to make a screenshot

@amanagr
Copy link
Member Author

amanagr commented Jul 20, 2022

P.S. Could you try to set up colors via HSLA spelling? or RGBA ? instead of hex...

For the steam recipient bar colors, we get hex values from the server. I can't use HSLA or RGBA there.

@terpimost
Copy link
Collaborator

P.S. Could you try to set up colors via HSLA spelling? or RGBA ? instead of hex...

For the steam recipient bar colors, we get hex values from the server. I can't use HSLA or RGBA there.

Yes, you have color as hex as a value for a stream, but in order to construct the transparency for the bar itself you have to convert specify the opacity value, right?

Are you saying that transparency value is controlled on a server side somehow?

@amanagr
Copy link
Member Author

amanagr commented Jul 25, 2022

@terpimost This should be working correctly now.

@terpimost
Copy link
Collaborator

I confirm that it works as discussed. The next step would be on my side to think how colors work a bit better. It would be a separate issue.

@alya
Copy link
Contributor

alya commented Jul 28, 2022

@amanagr if we can drop the background change from this PR, I can review it as well, and then we can plan to test on CZO.

@terpimost
Copy link
Collaborator

@amanagr
Copy link
Member Author

amanagr commented Aug 1, 2022

@amanagr if we can drop the background change from this PR, I can review it as well, and then we can plan to test on CZO.

I dropped the background change from it PR. There is still a lot to do here, some of it is in the WIP part of the PR:

I think we should finalize the steps we want to follow before I dive into any of these.

@amanagr
Copy link
Member Author

amanagr commented Aug 1, 2022

Some screenshots of current status.
Screenshot 2022-08-01 at 8 33 30 PM
Screenshot 2022-08-01 at 8 33 39 PM

@terpimost
Copy link
Collaborator

Hey @amanagr I'm checking colors for the same hashtag color from my example and from this PR:
image
It looks different.

@terpimost
Copy link
Collaborator

@zulipbot
Copy link
Member

zulipbot commented Oct 6, 2022

Heads up @amanagr, we just merged some commits that conflict with the changes you made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

@alya
Copy link
Contributor

alya commented Dec 6, 2022

I realized that I'm not sure what's meant by this point:

Stream color change process (which looks like should be a PR of its own).

Could you please clarify?

@alya
Copy link
Contributor

alya commented Dec 6, 2022

In general, what are the open questions here that we need to answer to unblock work on this?

I dropped the background change from it PR. There is still a lot to do here, some of it is in the WIP part of the PR:

I think we should finalize the steps we want to follow before I dive into any of these.

@alya
Copy link
Contributor

alya commented Dec 6, 2022

I think we could test on CZO without making any changes in the drafts overlay, so that can be a later step if it's not super easy to do.

@amanagr
Copy link
Member Author

amanagr commented Dec 7, 2022

@alya these are no longer a concern, thanks for your followup! I think I can safely close this PR since it is no longer relevant and has some changes that I wouldn't like to have too. I followed a completely different approach to resolve my concerns.

@amanagr amanagr closed this Dec 7, 2022
@alya
Copy link
Contributor

alya commented Dec 7, 2022

Ok, sounds good! Is there a current version of the PR that we should link with #22021, or is it local so far?

@amanagr
Copy link
Member Author

amanagr commented Dec 7, 2022

Yes local, I am working on it in this branch which I keep fairly updated. I expect to open a PR for it by this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants