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

Add navigation buttons below messages #31612

Closed
wants to merge 1 commit into from

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented Sep 16, 2024

Fixes #30405

design discussion: CZO Link

  • Fix puppeteer tests

  • We want to add navigation buttons below the bottom message in the view. They should make it more convenient to perform common navigation actions, and remove the need for banners to mark messages as read.

  • End, the "Scroll to bottom" button, and any other ways to jump to the bottom of the view should select this new navigation bar rather than the last message.

  • All the bottom of message feed banners with a "Mark as read" button should be removed as part of this issue.

  • The initial goal is to get to a testable prototype, as we expect to need to iterate on the UX once we can play with it. It's fine to leave any tricky implementation details for later if they don't seem critical (with notes on the PR).

Which buttons, when and where

  • We can start by making these all be medium-attention grey buttons (don't have to 100% match the design; whatever is easy to prototype).

Home view

  • - Label: Home view
  • - Placement: Left
  • - When to show: In all message views, unless you are already in your home view.
  • - What it does: Goes to your selected home view
  • - Tooltip:

If Esc goes to home view:

Go to your home view Esc
Otherwise:
Go to your home view +[

Mark as read

  • - Label: Mark as read
  • - What it does: Marks all messages in the current view as read.
  • - Placement: Center
  • - When to show: Replacing all the situations when we’d show a banner with this button; note that there's more than one type of banner.
  • - Tooltip:

1st line:

Mark as read
2nd line, if the banner was due to the setting (Never or Only in conversation views):
Messages are not automatically marked as read according to your settings.
2nd line, otherwise (same as the corresponding banner):
To preserve your reading state, this view does not mark messages as read.

Next unread conversation

  • Note that neither button is shown in other types of message views (search views, global feed).

This will resolve #18728.

DMs

  • - Label: Next unread conversation
  • - Placement: Right
  • - When to show: In DM conversations and the DM feed when there are unread DM conversations.
  • - What it does: P
  • - Tooltip:

Go to next unread DM conversation [P]

Channel feed and topic views

  • - Label: Next unread conversation
  • - Placement: Right
  • - When to show: In topic conversation views, /near views and channel feeds.
  • - What it does: N
  • - Tooltip:

Go to next unread topic [N]

Keyboard navigation behavior

Initially, the user will generally navigate to this bar as a whole (selected with the blue box). From there:

  • - In a conversation view, pressing Enter should start a reply to the current conversation, as it would on a message.

  • In non-conversation views, we can prototype with it doing nothing.

  • - Pressing Tab should select a button in the navigation bar.

  • - Pressing right arrow should select a button in the navigation bar.

  • We should probably remove the selection ring on the row as a whole at that point, but we'll have to test.

  • [ ]

  • - If "Mark as read" button is there, select that one.

  • - Otherwise, pick between "Home view" and "Next unread conversation"

  • depending on which one was used more recently, if both are present.

  • - Once one of the buttons is selected, right/left arrows and Tab/Shift+Tab should work to move between them, without cycling. (Maybe left from the leftmost and/or right from the rightmost would select the whole row again?)

Questions and follow-ups

  • Should we add any other actions, e.g., "Next unread followed topic"?

@amanagr amanagr marked this pull request as draft September 16, 2024 13:37
@zulip zulip deleted a comment from zulipbot Sep 16, 2024
@zulip zulip deleted a comment from zulipbot Sep 18, 2024
@amanagr amanagr force-pushed the navigation_buttons branch 2 times, most recently from 402725f to 3a70638 Compare September 18, 2024 12:06
@zulip zulip deleted a comment from zulipbot Sep 19, 2024
@amanagr amanagr marked this pull request as ready for review September 19, 2024 18:12
@timabbott
Copy link
Member

timabbott commented Sep 19, 2024

I played around with this a bit and it seems to mostly work. I have the following notes:

  • - There should be tooltip delay when hovering the new buttons; currently tooltips appear immediately. Maybe EXTRA_LONG_HOVER_DELAY, since they are mainly there to advertise keyboard shortcuts, not explain the button itself?
  • - Tooltips shouldn't appear on navigating to the buttons via the keyboard; we want them to only trigger on mouse hover, I think.
  • - The Left keyboard shortcut to edit last message doesn't work when the new widget is selected. We probably want that to work.
  • - If you highlight the new row when it has 3 buttons, and then hit "Right" to get to the center button, you can't reach the "Go to home" button via hitting "Left".

Didn't note other obvious bugs playing around with it.

web/src/filter.ts Outdated Show resolved Hide resolved
@zulip zulip deleted a comment from zulipbot Sep 20, 2024
@amanagr amanagr force-pushed the navigation_buttons branch 2 times, most recently from 86f86a0 to 8dbab58 Compare September 20, 2024 13:10
@zulip zulip deleted a comment from zulipbot Sep 20, 2024
@zulip zulip deleted a comment from zulipbot Sep 21, 2024
@amanagr
Copy link
Member Author

amanagr commented Sep 22, 2024

Pressing Tab should select a button in the navigation bar.

I think it works best not handle tab / shift + tab in the message view here too as we do for recent view and inbox.

We should probably remove the selection ring on the row as a whole at that point, but we'll have to test.

Are we sure we want to remove the selection ring?

@zulip zulip deleted a comment from zulipbot Sep 22, 2024
@amanagr
Copy link
Member Author

amanagr commented Sep 22, 2024

This is ready for another review.

@timabbott
Copy link
Member

Yeah I'm not sure if either of those last two suggestions make sense. I found one new clear bug:

  • If you're on the last actual message row, and press Down in a way that scrolls the feed, the post-scroll state ends up landing back on the last actual message row, not the navigation bar.

@zulip zulip deleted a comment from zulipbot Sep 23, 2024
@amanagr
Copy link
Member Author

amanagr commented Sep 23, 2024

Fixed that.

@timabbott timabbott added chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. labels Sep 24, 2024
@timabbott
Copy link
Member

Test-deployed this on chat.zulip.org for discussion and feedback.

@zulipbot
Copy link
Member

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.

@zulipbot
Copy link
Member

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.

@amanagr
Copy link
Member Author

amanagr commented Oct 21, 2024

Closing this since we are going in a slightly different direction and I don't want to lose this work in case we add something similar in the future.

@amanagr amanagr closed this Oct 21, 2024
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. deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. has conflicts size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add navigation buttons below messages Next unread topic/next unread DM conversation button
3 participants