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

message_list: Add buttons for different actions. #32091

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

message_list: Add buttons for different actions. #32091

wants to merge 1 commit into from

Conversation

amanagr
Copy link
Member

@amanagr amanagr commented Oct 22, 2024

@amanagr
Copy link
Member Author

amanagr commented Oct 22, 2024

I didn't spend a lot of time trying to find edge cases since we are still in the trial phase, so think of it this PR as prototype worth deploying to test out the feature.

@zulipbot zulipbot added size: XL area: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) priority: high UI experiment Design direction to be evaluated labels Oct 22, 2024
@zulipbot
Copy link
Member

Hello @zulip/design members, this pull request was labeled with the "UI experiment" label, so you may want to check it out!

@amanagr amanagr force-pushed the message_list_nav_buttons branch from 6ba6aea to e6bc2bb Compare October 22, 2024 18:23
@alya
Copy link
Contributor

alya commented Oct 22, 2024

Nice! I wonder if a shorter tooltip delay would be better, since the buttons are unlabeled now (maybe LONG rather than EXTRA_LONG).

@alya
Copy link
Contributor

alya commented Oct 22, 2024

It's definitely much more subtle than the previous version. I'd like to see this on CZO!

@alya alya added the chat.zulip.org review Added by maintainers when a PR requires chat.zulip.org testing to proceed. label Oct 22, 2024
timabbott
timabbott reviewed Oct 22, 2024
View reviewed changes
web/src/unread_ops.ts
@@ -544,7 +545,7 @@ export function process_unread_messages_event({
!message_lists.current.can_mark_messages_read() &&
message_lists.current.has_unread_messages()
) {
unread_ui.notify_messages_remain_unread();
message_list_navigation.update();
Copy link
Member

Choose a reason for hiding this comment

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

The refactor to have a message_list_navigation.update function has appeared in both versions of this; if we keep doing iterations on this, maybe we make a prep commit we can merge to reduce overall churn.

timabbott
timabbott reviewed Oct 22, 2024
View reviewed changes
web/src/ui_init.js
@@ -221,7 +222,7 @@ export function initialize_kitchen_sink_stuff() {
if (delta < 0 && message_viewport.at_rendered_top()) {
navigate.up();
} else if (delta > 0 && message_viewport.at_rendered_bottom()) {
navigate.down();
navigate.down(false, true);
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to pass these two options by name rather than positional values.

@amanagr amanagr force-pushed the message_list_nav_buttons branch from e6bc2bb to f661bcc Compare October 24, 2024 08:59
@timabbott timabbott added the deployed on chat.zulip.org Added by maintainers when a PR is currently being tested on chat.zulip.org. label Oct 24, 2024
@amanagr amanagr force-pushed the message_list_nav_buttons branch from f661bcc to 1636cf8 Compare November 10, 2024 02:31
@amanagr amanagr force-pushed the message_list_nav_buttons branch from 1636cf8 to af69642 Compare November 10, 2024 02:34
@amanagr
Copy link
Member Author

amanagr commented Nov 10, 2024

Updated to move navigation buttons to the right.

@amanagr amanagr force-pushed the message_list_nav_buttons branch from af69642 to 3f1048d Compare November 15, 2024 08:14
@amanagr amanagr force-pushed the message_list_nav_buttons branch from 3f1048d to baa5c5e Compare November 27, 2024 16:58
@amanagr amanagr force-pushed the message_list_nav_buttons branch from baa5c5e to 30efbef Compare December 5, 2024 07:12
@timabbott timabbott force-pushed the message_list_nav_buttons branch from 30efbef to 61d7b7d Compare December 11, 2024 01:28
@timabbott
Copy link
Member

Deploying back on chat.zulip.org again.

@amanagr amanagr force-pushed the message_list_nav_buttons branch from 61d7b7d to 930f4c4 Compare December 12, 2024 09:01
@amanagr
Copy link
Member Author

amanagr commented Dec 12, 2024

Pushed to resolve conflicts.

timabbott
timabbott reviewed Dec 12, 2024
View reviewed changes
web/styles/zulip.css
border-radius: 5px;
}

.message-list-navigation-button {
Copy link
Member

Choose a reason for hiding this comment

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

A good refactor that this makes me think of is extracting a message_feed.css with some of the outer containing classes like div.message-list, .empty_feed_notice, .sub-unsub-message, .date_row, etc.

This CSS would belong there; would be helpful for the goal of shrinking zulip.css. Not a blocker for this, but maybe a fine project if you're in the mood for that style of work.

timabbott
timabbott reviewed Dec 12, 2024
View reviewed changes
web/templates/tooltip_templates.hbs Outdated
@@ -107,14 +107,14 @@
</div>
</template>
<template id="all-message-tooltip-template">
<div class="views-tooltip-container" data-view-code="all_messages">
Copy link
Member

Choose a reason for hiding this comment

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

Could this data-view-code refactor be put in a prep commit or PR? I'm not sure what it's about, but it seems like it could be done earlier to help simplify review.

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #32728

timabbott
timabbott reviewed Dec 12, 2024
View reviewed changes
web/src/tippyjs.ts Outdated
trigger: "mouseenter",
delay: EXTRA_LONG_HOVER_DELAY,
appendTo: () => document.body,
});
Copy link
Member

Choose a reason for hiding this comment

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

I think these can be declared in message_list_navigation.ts; generally it's nicer to have all the code for a feature in one place, and so I've been trying to avoid just having every tooltip go in here when it could just live in its own module.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, moved!

timabbott
timabbott reviewed Dec 12, 2024
View reviewed changes
web/src/message_view.ts
@@ -880,6 +877,7 @@ export function fast_track_current_msg_list_to_anchor(anchor: string): void {
return msg_list_data.first()!.id;
},
});
message_lists.current.unfocus_navigation_bar();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand some of these changes in when we do the checks for updating these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we can make the focus / unfocus logic simple in the codebase but we need to nail down the experience we want here too.

@amanagr amanagr force-pushed the message_list_nav_buttons branch from 930f4c4 to 7bfb085 Compare December 16, 2024 10:39
@amanagr
Copy link
Member Author

amanagr commented Dec 16, 2024

Pushed mainly to just resolve conflicts. Didn't address any feedback in the design thread yet since the discussion didn't seem to have reached a conclusion.

@amanagr amanagr force-pushed the message_list_nav_buttons branch from 7bfb085 to 7535898 Compare December 16, 2024 14:03
@amanagr amanagr force-pushed the message_list_nav_buttons branch from 7535898 to da7912c Compare December 19, 2024 11:51
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: message feed (UI) Buttons/UI directly in the message feed (not popovers, etc.) 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. priority: high size: XL UI experiment Design direction to be evaluated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add navigation buttons below messages Next unread topic/next unread DM conversation button
4 participants
@amanagr @zulipbot @alya @timabbott