Is it a part of a stack, or are your diffs independent?

It's generally a part of the MessageActionButtons work, but I think it stands alone (was able to land the other diffs without this one)

It's weird to see a noun as an action. So either, the action could be create_sidebar or change type name to e.g. TooltipType.

Yeah you're right, I'll change this to TooltipType

This isn't a big issue but I feel that having this logic here reduces the readability. It would be better if we determined reply tooltip text while creating reply button, and sidebar text while creating sidebar button. @atul what do you think? I know that the proposed approach has its own issues, so we could also keep the current approach.

With the current approach, we have "one" tooltip component that conditionally appears above either the reply or sidebar buttons when they're hovered over. Because of this, we need to be able to dynamically change the text that appears within the tooltip depending on which icon is being pointed to.

We could create two separate tooltip components--one for each button--and set the text statically, but I'm not sure if that's a good idea? Based on the design it looks like we're going to have more buttons added to this row, so we'd have to create a separate tooltip for all of them? Maybe that's not crazy if we break the tooltip into its own component?

This sounds risky to toggle on enter. It would be safer to show on enter and hide on leave. It is consistent with the current implementation, but we can consider improving the solution.

Yeah it looks like it's not a "real" toggle since it just returns if tooltipVisible

if (tooltipVisible) {
  return;
}

So I guess really it's a show tooltip if not already visible...so what I really need to do is rename the functions to make their behavior more clear?

It feels like setActiveTooltip should be a part of showTooltip function. Currently, we need to call two functions and it's harder to know what's actually happening. If we had a function showTooltip(TooltipType, Event), then we could call it showTooltip('sidebar', event) which hides an implementation detail that we're storing the state in two separate places.

I think we still need two callbacks to pass into each onMouseEnter. If we were to include the type as an argument to show tooltip, we'd still need

showSidebarTooltip = React.useCallback((event) => showTooltip('sidebar', event));
showReplyTooltip = React.useCallback((event) => showTooltip('reply', event));

Can still make that change if we think it would be an improvement

But the fact that we're storing the state separated might be another thing which we can consider improving. I'm not sure if we need tooltipVisible at all: we can change the code slightly so that when activeTooltip === null we don't show a tooltip, and when activeTooltip !== null we show activeTooltip. What do you think?

Got rid of tooltipVisible state! Thanks for that suggestion, it's definitely cleaner

https://linear.app/comm/issue/ENG-799/message-tooltip-position-is-incorrect -- @atul assuming you're working on this. Could you update the linear task if you're taking this issue on?

https://linear.app/comm/issue/ENG-799/message-tooltip-position-is-incorrect

This diff is part of https://linear.app/comm/issue/ENG-794/create-sidebar-pointer-icon-is-borked and has to do with the message action buttons. Looks like ENG-799 is a separate issue regarding the timestamp tooltip.

I could take a look after this issue is completed? I guess they're kind of related