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

Navbar: Describe views in top navbar. #29824

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Navbar: Describe views in top navbar. #29824

wants to merge 1 commit into from
+74 −1

Conversation

nimishmedatwal
Copy link
Collaborator

@nimishmedatwal nimishmedatwal commented Apr 23, 2024 •
edited

Adds description in views styled like stream descriptions along with adding a help center link to the appropriate page at the end of each description.

CZO thread

Fixes: #29769

Screenshots and screen captures:

image
image
image
image
image

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@zulipbot
Copy link
Member

Hello @zulip/server-onboarding members, this pull request was labeled with the "area: onboarding" label, so you may want to check it out!

@nimishmedatwal nimishmedatwal force-pushed the issue#29769 branch 2 times, most recently from 3e02e8c to 4eb2ad1 Compare April 23, 2024 18:16
@alya
Copy link
Contributor

alya commented Apr 23, 2024

Thanks! This does not work well when narrowing the window:

Screenshot 2024-04-23 at 11 59 41
Screenshot 2024-04-23 at 11 59 11

@alya
Copy link
Contributor

alya commented Apr 23, 2024

We can let the question mark disappear with the rest of the text when everything doesn't fit.

@alya
Copy link
Contributor

alya commented Apr 23, 2024

I also think the question mark is spaced a bit too far to the right.

@alya
Copy link
Contributor

alya commented Apr 23, 2024

I'm feeling good about the strings after seeing them in the UI.

@nimishmedatwal
Copy link
Collaborator Author

nimishmedatwal commented Apr 23, 2024 •
edited

@alya for me the help icon is disappearing when narrowing the windows. (I am using brave browser in windows)

image

Recording.2024-04-24.004546.mp4

EDIT: I've tested in edge browser and on safari as well and it's working fine for me
image

@nimishmedatwal
Copy link
Collaborator Author

nimishmedatwal commented Apr 23, 2024 •
edited

@alya I've also corrected the spacing of help-icon.
image

I personally think it would look better if we remove the full stop in the end and the margin stays as it is like this:

image

@alya alya mentioned this pull request Apr 23, 2024
Closed
13 tasks
@alya
Copy link
Contributor

alya commented Apr 23, 2024

OK, I dunno. I tested in Chrome on a Mac.

@karlstolley
Copy link
Contributor

I've tested on both Firefox and Chrome on Mac, and I'm not able to reproduce the problem @alya is seeing with the help icon. Everything in testing looks good on my end, in other words.

@karlstolley
Copy link
Contributor

Everything here LGTM. Probably worth some time on CZO, just because there is this head-scratcher with the help icon. I'll mark it as such, but @timabbott will make whatever seems to be the better call here.

@karlstolley karlstolley added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label Apr 24, 2024
timabbott
timabbott reviewed Apr 25, 2024
View reviewed changes
web/src/message_view_header.ts Outdated
zulip_icon: "recent",
link: $t({defaultMessage: "/help/recent-conversations"}),
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Don't tag links for translation; that doesn't make any sense -- the URL is the same for everyone regardless of language. These should just be fixed URLs.

timabbott
timabbott reviewed Apr 25, 2024
View reviewed changes
web/src/message_view_header.ts Outdated
@@ -59,6 +71,21 @@ function get_message_view_header_context(filter: Filter | undefined): MessageVie
title,
is_spectator: page_params.is_spectator,
});
if (title === "Mentions") {
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You can't check the titles this way, because they ARE tagged for translation -- so it'll be different in other languages. I think you need filter.get_title to be doing these computations, and this function just displaying what it passed through.

Take a read through https://zulip.readthedocs.io/en/latest/translating/internationalization.html.

timabbott
timabbott reviewed Apr 25, 2024
View reviewed changes
web/templates/navbar_icon_and_title.hbs Outdated
{{/if}}
</span>
{{/if}}

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Don't add extra blank lines at the end of files like this.

@zulipbot zulipbot added size: L and removed size: M labels Apr 26, 2024
@nimishmedatwal
Copy link
Collaborator Author

nimishmedatwal commented Apr 26, 2024 •
edited

@timabbott thank you for your review!
I've fixed all the issues you pointed out, instead of changing get_title I've created a new function get_description which returns the description and help center link. I've also written tests for that function and I think it's ready for another review.

@nimishmedatwal
navbar: Describe views in top navbar.
91d8724 
Adds description in views styled like
stream descriptions also adds a help center
link to the appropriate page at the end of
each description.
Fixes zulip#29769.
@timabbott timabbott added the deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. label May 2, 2024
@timabbott
Copy link
Sponsor Member

The "combined feed" description seems to not be working on chat.zulip.org.

@zulipbot
Copy link
Member

zulipbot commented May 3, 2024

Heads up @nimishmedatwal, 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.

@timabbott timabbott added the completion candidate PRs with reviews that may unblock merging label May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timabbott timabbott timabbott left review comments

Assignees
No one assigned
Labels
area: navbar area: onboarding chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. completion candidate PRs with reviews that may unblock merging deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. has conflicts priority: high size: L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Describe views in top navbar
5 participants
@nimishmedatwal @zulipbot @alya @karlstolley @timabbott