From 8cb291baa446a9fcf4d7fcfec16d3dc7d416277e Mon Sep 17 00:00:00 2001 From: Marshall Bowers Date: Tue, 9 Jan 2024 18:39:29 -0500 Subject: [PATCH] Take a different approach to rendering channel buttons (#3991) This PR changes the approach we're using to render the channel buttons to use a more straightforward (and less hacky) approach. ### Motivation Even with the variety of hacks that were employed to make the current approach work, there are still a number of issues with the current solution: - Hovering in the empty space to the left of a channel doesn't correctly apply the hover background to the container for the channel buttons - Hovering to the very right of the collab panel (right on top of the drag handle) causes the channel button container to apply its hover background without applying it to the rest of the row - The buttons would still get pushed off to the right by the indent space in the channel tree, resulting in jagged indicators at small sizes Additionally, the rectangular background placed behind the channel buttons still didn't look great when it overlapped with the channel names. ### Explanation For these reasons, I decided to explore a simpler approach that addresses these issues, albeit with some tradeoffs that I think are acceptable. We now render the absolutely-positioned button container as a sibling element of the `ListItem`. This is to avoid issues with the container getting pushed around based on the contents of the `ListItem` rather than staying absolutely positioned at the end of the row. We also have gotten rid of the background for the button container, and now rely on the background of the individual `IconButton`s to occlude the channel name behind them when the two are overlapping. Here are some examples of the new UI in various configurations: #### When the channel entry is hovered Screenshot 2024-01-09 at 6 15 24 PM #### Overlapping with the channel name Screenshot 2024-01-09 at 6 15 43 PM #### Narrow collab panel Screenshot 2024-01-09 at 6 16 07 PM ### Tradeoffs The new approach comes with the following tradeoffs that I am currently aware of: The occlusion can look a little weird when the icons are in the middle of a channel name (as opposed to fully occluding the end of the channel name): Screenshot 2024-01-09 at 6 24 32 PM Hovering one of the icons causes the icon to be hovered instead of the row: Screenshot 2024-01-09 at 6 31 38 PM Release Notes: - Improved the way channel buttons are displayed. --- crates/collab_ui/src/collab_panel.rs | 139 ++++++++++----------------- 1 file changed, 49 insertions(+), 90 deletions(-) diff --git a/crates/collab_ui/src/collab_panel.rs b/crates/collab_ui/src/collab_panel.rs index df8f2a251f..b9946d6cac 100644 --- a/crates/collab_ui/src/collab_panel.rs +++ b/crates/collab_ui/src/collab_panel.rs @@ -2228,47 +2228,6 @@ impl CollabPanel { None }; - let button_container = |cx: &mut ViewContext| { - h_stack() - .absolute() - // We're using a negative coordinate for the right anchor to - // counteract the padding of the `ListItem`. - // - // This prevents a gap from showing up between the background - // of this element and the edge of the collab panel. - .right(rems(-0.5)) - // HACK: Without this the channel name clips on top of the icons, but I'm not sure why. - .z_index(10) - .bg(cx.theme().colors().panel_background) - .when(is_selected || is_active, |this| { - this.bg(cx.theme().colors().ghost_element_selected) - }) - }; - - let messages_button = |cx: &mut ViewContext| { - IconButton::new("channel_chat", IconName::MessageBubbles) - .icon_size(IconSize::Small) - .icon_color(if has_messages_notification { - Color::Default - } else { - Color::Muted - }) - .on_click(cx.listener(move |this, _, cx| this.join_channel_chat(channel_id, cx))) - .tooltip(|cx| Tooltip::text("Open channel chat", cx)) - }; - - let notes_button = |cx: &mut ViewContext| { - IconButton::new("channel_notes", IconName::File) - .icon_size(IconSize::Small) - .icon_color(if has_notes_notification { - Color::Default - } else { - Color::Muted - }) - .on_click(cx.listener(move |this, _, cx| this.open_channel_notes(channel_id, cx))) - .tooltip(|cx| Tooltip::text("Open channel notes", cx)) - }; - let width = self.width.unwrap_or(px(240.)); div() @@ -2326,58 +2285,58 @@ impl CollabPanel { .child( h_stack() .id(channel_id as usize) - // HACK: This is a dirty hack to help with the positioning of the button container. - // - // We're using a pixel width for the elements but then allowing the contents to - // overflow. This means that the label and facepile will be shown, but will not - // push the button container off the edge of the panel. - .w_px() .child(Label::new(channel.name.clone())) .children(face_pile.map(|face_pile| face_pile.render(cx))), - ) - .end_slot::
( - // If we have a notification for either button, we want to show the corresponding - // button(s) as indicators. - if has_messages_notification || has_notes_notification { - Some( - button_container(cx).child( - h_stack() - .px_1() - .children( - // We only want to render the messages button if there are unseen messages. - // This way we don't take up any space that might overlap the channel name - // when there are no notifications. - has_messages_notification.then(|| messages_button(cx)), - ) - .child( - // We always want the notes button to take up space to prevent layout - // shift when hovering over the channel. - // However, if there are is no notes notification we just show an empty slot. - notes_button(cx) - .when(!has_notes_notification, |this| { - this.visible_on_hover("") - }), - ), - ), + ), + ) + .child( + h_stack() + .absolute() + .right(rems(0.)) + .h_full() + // HACK: Without this the channel name clips on top of the icons, but I'm not sure why. + .z_index(10) + .child( + h_stack() + .h_full() + .gap_1() + .px_1() + .child( + IconButton::new("channel_chat", IconName::MessageBubbles) + .style(ButtonStyle::Filled) + .size(ButtonSize::Compact) + .icon_size(IconSize::Small) + .icon_color(if has_messages_notification { + Color::Default + } else { + Color::Muted + }) + .on_click(cx.listener(move |this, _, cx| { + this.join_channel_chat(channel_id, cx) + })) + .tooltip(|cx| Tooltip::text("Open channel chat", cx)) + .when(!has_messages_notification, |this| { + this.visible_on_hover("") + }), ) - } else { - None - }, - ) - .end_hover_slot( - // When we hover the channel entry we want to always show both buttons. - button_container(cx).child( - h_stack() - .px_1() - // The element hover background has a slight transparency to it, so we - // need to apply it to the inner element so that it blends with the solid - // background color of the absolutely-positioned element. - .group_hover("", |style| { - style.bg(cx.theme().colors().ghost_element_hover) - }) - .child(messages_button(cx)) - .child(notes_button(cx)), - ), + .child( + IconButton::new("channel_notes", IconName::File) + .style(ButtonStyle::Filled) + .size(ButtonSize::Compact) + .icon_size(IconSize::Small) + .icon_color(if has_notes_notification { + Color::Default + } else { + Color::Muted + }) + .on_click(cx.listener(move |this, _, cx| { + this.open_channel_notes(channel_id, cx) + })) + .tooltip(|cx| Tooltip::text("Open channel notes", cx)) + .when(!has_notes_notification, |this| { + this.visible_on_hover("") + }), + ), ), ) .tooltip(|cx| Tooltip::text("Join channel", cx))