Expensify chat - web/desktop, when hovering over a user's name anywhere, show their primary login details #1490

laurenreidexpensify · 2021-02-17T11:28:15Z

If you haven’t already, check out our contributing guidelines for onboarding!

Deliverable:
On web/desktop, when hovering over a user's name anywhere, show their primary login. This includes:

In the header of a DM (whether single or group)
In the chat itself (ie, the header of a new comment line)
The chat row in the LHN (whether single or group)
The chat row in the search results (whether single or group)

Couple of other notes:

When the full list of names is hidden behind the ... ellipsis for Group DMs in the LHN/search results, I'd suggest we show a grouped list of all the participants' primaryLogins on hover of the ellipsis
for users that have phone numbers as primaryLogins, let's drop the @expensify.sms portion, so it's displayed on hover as: +447814266906 and not +447814266906@expensify.sms

https://www.upwork.com/jobs/~0146d36b55605d9064

parasharrajat · 2021-02-17T14:35:27Z

Proposal

We will use the Tooltip component to wrap the report Titles.
Due to the header text is taking the full width of its parent we need to wrap such text into a wrapper view with flex-direction: column to adjust the Tooltip position.

 <View style={{flexDirection: 'column', alignItems: 'flex-start'}}>
            <Tooltip text={props.title}>
                <Text numberOfLines={2} style={[styles.headerText, props.textSize === 'large' && styles.textLarge]}>
                    {props.title}
                </Text>
            </Tooltip>
        </View>

for demo purposes I have added styling here.

Now the issue is that Tooltip adds two wrappers on the nodes which block text-clipping.

There are two things we need to care of.

a) Remove the Hoverable component's wrapper view in this way

  const child = _.isFunction(this.props.children)
           ? this.props.children(this.state.isHovered)
           : this.props.children;

       return React.cloneElement(React.Children.only(child), {
           onMouseEnter: this.toggleHoverState,
           onMouseLeave: this.toggleHoverState,
       });

b) Next, we need to update the tooltip component to take style props so that we can give max-width: 100% to allow clipping of text.

We need to allow attaching the tooltip to the body of the page so that It's not hidden by other page elements with some standard z-index.
we use a container prop which decides the placement of the Tooltip Node to the Body element.

Part 2

we modify the function here and add another property into the options object with the value of the report name without the @expensify.sms text. Now we will use this for Tooltip text.
https://github.com/Expensify/Expensify.cash/blob/88a0c5e8f137b08c273aa7926e5326a621dd8eca/src/libs/OptionsListUtils.js#L78-L109

roryabraham · 2021-02-19T20:41:14Z

Due to the header text is taking the full width of its parent we need to wrap such text into a wrapper view with flex-direction: column to adjust the Tooltip position.

I'm not sure I follow why this is necessary. The tooltip should be centered on its children, so if the child is a text element it should be centered on the text.

Now the issue is that Tooltip adds two wrappers on the nodes which block text-clipping

Again, could you explain this problem in a bit more detail?

Next, we need to update the tooltip component to take style props so that we can give max-width: 100% to allow clipping of text.

Why not just give the tooltip a default style of max-width: 100%?

We use a container prop which decides the placement of the Tooltip Node to the Body element.

I've experienced the z-index problem you've describe before (and come up with a different solution), but I'm generally unclear on what you're proposing here.

roryabraham · 2021-02-19T20:43:22Z

Generally it looks like you've thought out your proposal pretty thoroughly, but you maybe just need to do a better job explaining the base solution. Then for each additional change, make it more clear the problem you're trying to solve, and we can then discuss the best solution.

parasharrajat · 2021-02-19T21:54:58Z

@roryabraham . My bad explaining it.

I'm not sure I follow why this is necessary. The tooltip should be centered on its children, so if the child is a text element it should be centered on the text.

https://github.com/Expensify/Expensify.cash/blob/380a148fdd47ce4c37e07ce647e1d9d31af8eb88/src/components/Header.js#L18-L24
the Text element is rendered as a div on the web which takes the full available width in flex parent. so my bad here. we can simply set the parent View as flex-direction: row and set the tooltip max-width: 100%.

 <View style={[styles.flex1, styles.flexRow]}>
        <Tooltip text={props.title}>
            <Text numberOfLines={2} style={[styles.headerText, props.textSize === 'large' && styles.textLarge]}>
                {props.title}
            </Text>
        </Tooltip>
   </View>

Now the issue is that Tooltip adds two wrappers on the nodes which block text-clipping

it adds two wrapper divs, for the text to clip we need to set max-width on the parent wrapper. Max-width should be given to the immediate child of the flex container, it would be the wrapper by Hoverable component. I don't think it's a good idea to give the Hoverable component any styling as it can be used in other places as well. Check the Inspected HTML around the Text(Tooltip is added here)

And I think we can remove the Hoverable component's View in the following way.

  const child = _.isFunction(this.props.children)
           ? this.props.children(this.state.isHovered)
           : this.props.children;

       return React.cloneElement(React.Children.only(child), {
           onMouseEnter: this.toggleHoverState,
           onMouseLeave: this.toggleHoverState,
       });

We use a container prop that decides the placement of the Tooltip Node to the Body element.

I meant that we need to attach the node of the tooltip to the body element in the DOM otherwise it will be hidden from other elements on the page so we use a prop container(standard name among frameworks) that decides whether we need to attach the Tooltip inline or to the Body of Page.

roryabraham · 2021-02-19T22:45:28Z

Okay, thanks for the explanation, proposal looks good. Just a couple things:

And I think we can remove the Hoverable component's View in the following way.

For what its worth, this was the original implementation I used for the Hoverable component, but I was convinced to not do it that way in order to reduce complexity and because there's no real reason to enforce that it has only one child. Now that there's a clear reason to not add an additional view, I might suggest you do it like this (so it can still have multiple children if desired):

return React.children.map(children, child => {
    const childWithHoverState = _.isFunction(child)
        ? child(this.state.isHovered)
        : child;
    return React.cloneElement(childWithHoverState, {
        onMouseEnter: this.toggleHoverState,
        onMouseLeave: this.toggleHoverState,
    });
});

Not certain that's 100% correct, but you get the idea hopefully.

I meant that we need to attach the node of the tooltip to the body element in the DOM otherwise it will be hidden from other elements on the page so we use a prop container(standard name among frameworks) that decides whether we need to attach the Tooltip inline or to the Body of Page.

I'm a bit conflicted about this because, while it will work on web, I don't think it will work on native platforms. We want to keep this app 99.9999% cross-platform where possible. So I'd prefer if you just try to work around this by giving a higher z-index to the parent elements that contain a tooltip that's covered up by neighboring elements, rather than trying to attach the tooltip to the root.

If that really can't work for some reason, then I think I'd prefer to see this approach:

Split up the tooltip into a web and native implementation,
On web, always attach it to the root element, rather than adding a new prop
On native, just have it do nothing for now (because we don't have a Hoverable that does anything on native yet anyways).

parasharrajat · 2021-02-19T22:57:42Z

because there's no real reason to enforce that it has only one child

I don't think that will there be a case where we need to add two direct children to the Hoverable. Then it would mean that hovering over two at a time, Not Good in Tree-based Dom(Hover state should link to only one node in DOM context https://html.spec.whatwg.org/multipage/semantics-other.html#selector-hover). There will always be a parent who gets hover if we need both children to have the hover effect. Or, Add hoverable to both children separately. CC: @marcaaron

If that really can't work for some reason, then I think I'd prefer to see this approach:

I agree. There will be two implementations one for native (Empty wrapper)and one for the web.

marcaaron · 2021-02-20T00:01:24Z

Sorry just catching up but I'm still stuck on this statement and wondering if we are overthinking the problem to compensate for this assumption here...

I don't think it's a good idea to give the Hoverable component any styling as it can be used in other places as well

It makes sense to not give it a style by default, but if you need to set some styles on the View to achieve some result it seems reasonable to allow. If we pass a container style as a prop to Tooltip can this solve the issue?

parasharrajat · 2021-02-20T00:15:23Z

Yeah. @marcaaron. We just need to set some styling for the text to clip in most of the cases.
So it's better to pass props for optional styling. Needs a green signal here.

laurenreidexpensify · 2021-02-24T13:23:47Z

@marcaaron is @parasharrajat good to go here?

laurenreidexpensify · 2021-02-26T16:18:16Z

@marcaaron @roryabraham bump when you get a moment today thanks

marcaaron · 2021-02-26T18:04:43Z

LGTM. We can hash the rest out in reviews in any case.

laurenreidexpensify · 2021-03-03T10:32:03Z

hi @parasharrajat is there a PR for this one yet?

parasharrajat · 2021-03-03T11:44:49Z

Soon, I will add one. @laurenreidexpensify

parasharrajat · 2021-03-04T14:39:52Z

Few Questions:

How do we need to show the Logins in the Group DMs in LHN.

laurenreidexpensify · 2021-03-04T15:12:05Z

@trjExpensify @shawnborton

trjExpensify · 2021-03-04T15:39:09Z

👋

We should show the primaryLogin of the user in the Group DM you're hovering over
In the case where there are many participants and so truncated with the ... ellipsis, when hovering over that ellipsis, we should show a list of all the participants of the Group DM.

parasharrajat · 2021-03-04T17:00:13Z

@trjExpensify Thanks. But let me be clear so that we are on the same page.

primaryLogin => Email and not the Report Title.

So

Show the tooltip for the single user Chats while hovering the title.
Show the tooltip on the Ellipse for Group chats only if the text is truncated.
Show the Tooltip with Points 1 and 2 on the search Page.
Show the Tooltip with Points 1 and 2 on the Report Header.
Show the Tooltip for the comment author.

Is the above behavior Correct?

trjExpensify · 2021-03-04T20:31:54Z

That's almost it! In addition to showing the full list of participants on a Group DM when hovering over an ... - we were also looking to achieve the following for Group DMs in the LHN:

Is that possible?

parasharrajat · 2021-03-04T21:22:32Z

Yup, it is.

trjExpensify · 2021-03-04T22:06:23Z

Awesome, thanks!

parasharrajat · 2021-03-05T19:38:23Z

@marcaaron
So, I have tried two ways to show the user's name tooltip on the Report Title but both of them have their limitations.
First:

(
                <Text style={[styles.optionDisplayName, style]} numberOfLines={1} ref={this.ref}>
                    {_.map(option.participantsList, (participant, index) => (
                        <Fragment key={index}>
                            <Tooltip key={index} text={participant.login} containerStyle={styles.dInline}>
                                <Text>
                                    {participant.displayName}
                                </Text>
                            </Tooltip>
                            {index < option.participantsList.length - 1 ? <Text>{', '}</Text> : null}
                        </Fragment>
                    ))}
                </Text>
            );

Second:

<Tooltip text={activeParticipant}>
                   <Text style={[styles.optionDisplayName, style]} numberOfLines={1}>
                       {_.map(option.participantsList, (participant, index) => (
                           <>
                               <Hoverable
                                   onHoverIn={() => this.selectParticipant(index)}
                                   containerStyle={styles.dInline}
                               >
                                   <Text>
                                       {participant.displayName}
                                   </Text>
                               </Hoverable>
                               {index < option.participantsList.length - 1 ? <Text>{', '}</Text> : null}
                           </>
                       ))}
                   </Text>
               </Tooltip>

The first one shows a separate tooltip for each name but we have to put display: inline to the containers added by tooltip for the ellipsis to be shown which causes the Tooltip to show at the start of the user name but not in the middle. On the other hand, the second one has one tooltip whose text change based on the user name we are hovering over which causes static change in the text but no animation when moving from one user name to another on Group DM
I have added some logic to detect the ellipsis and show the Tooltip over it which is as follows:

 {
                       isEllipsisActive
                           ? (
                           // visually hidden but it is slightly right from the original ellipsis location
                           
                               <View style={styles.optionDisplayNameTooltipEllipsis}>
                                   <Tooltip text={option.tooltipText}>
                                       <Text>...</Text>
                                   </Tooltip>
                               </View>
                           ) : null
                   }

Now, I need some help to determine if I am on the right path or not.

roryabraham · 2021-03-06T01:10:04Z

So just so I understand the effect of each of these approaches:

The Tooltip appears left-aligned with individual words, rather than centered in the middle of the word.
The Tooltip is centered on the full title, but does not move to be centered on individual words.

Is that correct?

we have to put display: inline to the containers added by tooltip for the ellipsis to be shown

Have you dug into why this is true? Would it be possible to create a Text wrapper that supports numberOfLines correctly but also uses allows flex children? Also, maybe check out this article because it may be related / lead you to another workaround!

parasharrajat · 2021-03-06T08:26:58Z

Thanks for the article, I am going dig more on it.

laurenreidexpensify · 2021-03-09T15:14:56Z

Hi @parasharrajat any luck yet? Can you post a little update of where you're at with the research? Thanks

parasharrajat · 2021-03-09T15:30:33Z

Yeah. @laurenreidexpensify I am going to make a screencast of all the approaches. I think we have to pick one which best suited or We have to make changes to the Tooltip.

roryabraham · 2021-03-09T16:08:15Z

@parasharrajat Did min-width on the flex children help with the text wrapping issue?

parasharrajat · 2021-03-09T17:19:25Z

@roryabraham so putting min-width:0; is fine. But the issue is that if there are multiple items, ellipsis show on multiple names, I guess due to the flex container try to shoe all the children and every child is shrinkable.

Another shrinking behavior was that it shrinks the complete item. and just show ... instead for a particular child even if there is space left.

So we just need to find a way to shrink the least children from the last. We need to put flex-shrink only on the last children that should be shrunk.

I am recording this So we have a clear picture.

With Flex Items and setting the Tooltip containers as

flexDirection: 'row',
                                    flexBasis: 'auto',
                                    flexShrink: 1,

With Inline Items and setting the Tooltip containers as display Inline.

screen-.2.mp4

As you clearly see that we get the desired text wrapping with display inline but the tooltip position is still calculated based on the full width of the content wrapper. On the other hand, Flex items does has text wrapping but multiple items are shrunk at a time to keep all the items visible(default behaviour by browsers) check the note.

Any input would help.

What I think at the moment?

Can we use a Grid layout? I think it's possible with grid layout. check here

parasharrajat · 2021-03-11T13:03:28Z

@roryabraham Any input for the comment above.
I think what I can do is

Use inline elements, as I mentioned above, and then shift the tooltip to the left using

  // Any additional amount to manually adjust the horizontal position of the tooltip.
    // A positive value shifts the tooltip to the right, and a negative value shifts it to the left.
shiftHorizontal: PropTypes.number,

    // Any additional amount to manually adjust the vertical position of the tooltip.
    // A positive value shifts the tooltip down, and a negative value shifts it up.
 shiftVertical: PropTypes.number,

But I need to know how to calculate these exactly.

roryabraham · 2021-03-11T19:15:17Z

Sorry for just getting back to this @parasharrajat, but looking at the video in the last comment, it looks like the tooltip is centered on the word, and the text-shrinking is happening as expected? What is the problem with this approach?

parasharrajat · 2021-03-11T19:18:19Z

@roryabraham In the video, you see that the tooltip should be rendered over the shrunk title (over the Sidebar) but it is being rendered on the center of the whole text underneath (which goes beyond the sidebar). But I have managed to show it at the correct location using the horizontalShift. Just preparing the PR for review.

roryabraham · 2021-03-11T19:21:48Z

Okay sounds good, please feel free to add me as a reviewer 👍

laurenreidexpensify · 2021-03-17T11:35:45Z

@parasharrajat is there a PR yet for this one? Thanks

parasharrajat · 2021-03-17T13:05:21Z

@laurenreidexpensify Yeah, I made one couple days back which is under review. #1632

laurenreidexpensify · 2021-03-17T13:10:08Z

Awesome! Thanks!!

roryabraham · 2021-03-24T16:15:27Z

We've had a minor issue with the solution here, in that tooltips are appearing on mobile web.

parasharrajat · 2021-03-24T16:23:03Z

Sure. I will pick this one. Let me think about the solution and get back to you, here.

roryabraham · 2021-03-24T16:29:17Z

@parasharrajat One potential solution is to:

set up a listener in Hoverable/index.js for the touchstart event which sets a property on the class called hoverDisabled
Update setIsHovered so that it doesn't set the hover state to true if hover is disabled:

setIsHovered(isHovered) {
    if (isHovered !== this.state.isHovered && !(isHovered && this.hoverDisabled)) {
        this.setState({isHovered}, isHovered ? this.props.onHoverIn : this.props.onHoverOut);
    }
}

Set up a listener in for the touchend event to set hoverDisabled back to false. That way it's future-proof and we can still offer hover capabilities on hybrid devices with mouse and touch interfaces.

roryabraham · 2021-03-24T16:33:22Z

Another potential solution (less fleshed out) would be to:

Give the tooltip display animation a delay, so that it only displays after an element has been hovered for 500ms.
onHoverOut, cancel the animation
Set up some global function like hideAllTooltips that we can use to hide any & all tooltips whenever there is a navigation change. That will hide tooltips when modals appear, a new screen is navigated to, etc...

This solution comes with the added bonus that the animation delay (in my opinion) would improve the UX of tooltips, because right now they feel a bit over-zealous.

roryabraham · 2021-03-24T16:33:46Z

We could also do both 😃

parasharrajat · 2021-03-24T16:40:59Z

Do you want the tooltips to be shown on the mobile web?

If no, then I would say just disable the Tooltips for Mobile Web.
Otherwise, I would pick the first solution over the second. It would save a lot of computation.
I like the Idea of animation delay. I guess we will cover this in #1778.

roryabraham · 2021-03-24T16:52:46Z

Do you want the tooltips to be shown on the mobile web

I think I prefer the first solution I outlined, because it allows us to have tooltips on hybrid devices (which are increasingly common).

marcaaron · 2021-03-24T17:29:19Z

This solution comes with the added bonus that the animation delay (in my opinion) would improve the UX of tooltips, because right now they feel a bit over-zealous.

Agree. And while we are going back in can we also please tone down the scaling effect here? I think a simple fade would look a lot cleaner, but curious to get @shawnborton's thoughts.

parasharrajat · 2021-03-24T18:13:19Z

@roryabraham Unfortunately. First Solution is not working as expected.Read here why #1 - Clicking and Tapping - the "Natural" Order of Things. Touchend fires before mouseEnter thus it resets the state we want.

But I have adjusted it.
We have to replace touchend with touchmove and set the hoverDisabled to false in the setIsHovered when isHovered is false

shawnborton · 2021-03-24T18:36:22Z

Agree. And while we are going back in can we also please tone down the scaling effect here? I think a simple fade would look a lot cleaner, but curious to get @shawnborton's thoughts.

That works for me. Something nice and clean.

parasharrajat · 2021-03-24T18:46:13Z

Okay, @shawnborton @marcaaron. I will refer this to a new PR shortly.

roryabraham · 2021-03-24T19:21:41Z

I personally think that just the delay will be enough to alleviate the "busyness" that the tooltips have right now. I kind of like the scaling effect.

roryabraham · 2021-03-24T19:30:31Z

Reopening since @parasharrajat has another PR coming for the tooltip delay

laurenreidexpensify · 2021-03-25T09:17:58Z

Okay - because of the regression with tooltips are appearing on mobile web, payment will now be 7 days from yesterday's deploy - so 31 March.

marcaaron assigned parasharrajat Feb 26, 2021

parasharrajat mentioned this issue Mar 4, 2021

Added Tooltips to the report users' titles. #1632

Merged

3 tasks

tgolen closed this as completed in #1632 Mar 19, 2021

roryabraham reopened this Mar 24, 2021

parasharrajat mentioned this issue Mar 24, 2021

fixed tooltip shows up on Mobile Web while navigating the reports #2044

Merged

1 task

roryabraham closed this as completed in #2044 Mar 24, 2021

roryabraham reopened this Mar 24, 2021

parasharrajat mentioned this issue Mar 24, 2021

Tooltip modifications #2056

Merged

1 task

marcaaron closed this as completed in #2056 Mar 25, 2021

huzaifa-99 mentioned this issue May 31, 2023

[HOLD for payment 2023-07-14] [$1000] The tooltip disappears when the "Copy to clipboard" option is clicked multiple times. #19693

Closed

6 tasks

Expensify chat - web/desktop, when hovering over a user's name anywhere, show their primary login details #1490

Expensify chat - web/desktop, when hovering over a user's name anywhere, show their primary login details #1490

Comments

laurenreidexpensify commented Feb 17, 2021 • edited Loading

parasharrajat commented Feb 17, 2021 • edited Loading

Proposal

There are two things we need to care of.

Part 2

roryabraham commented Feb 19, 2021

roryabraham commented Feb 19, 2021

parasharrajat commented Feb 19, 2021 • edited Loading

roryabraham commented Feb 19, 2021

parasharrajat commented Feb 19, 2021 • edited Loading

marcaaron commented Feb 20, 2021 • edited Loading

parasharrajat commented Feb 20, 2021 • edited Loading

laurenreidexpensify commented Feb 24, 2021

laurenreidexpensify commented Feb 26, 2021

marcaaron commented Feb 26, 2021

laurenreidexpensify commented Mar 3, 2021

parasharrajat commented Mar 3, 2021

parasharrajat commented Mar 4, 2021

laurenreidexpensify commented Mar 4, 2021

trjExpensify commented Mar 4, 2021

parasharrajat commented Mar 4, 2021

trjExpensify commented Mar 4, 2021

parasharrajat commented Mar 4, 2021

trjExpensify commented Mar 4, 2021

parasharrajat commented Mar 5, 2021 • edited by roryabraham Loading

roryabraham commented Mar 6, 2021

parasharrajat commented Mar 6, 2021

laurenreidexpensify commented Mar 9, 2021

parasharrajat commented Mar 9, 2021

roryabraham commented Mar 9, 2021

parasharrajat commented Mar 9, 2021 • edited Loading

I am recording this So we have a clear picture.

What I think at the moment?

parasharrajat commented Mar 11, 2021 • edited Loading

roryabraham commented Mar 11, 2021 • edited Loading

parasharrajat commented Mar 11, 2021 • edited Loading

roryabraham commented Mar 11, 2021

laurenreidexpensify commented Mar 17, 2021

parasharrajat commented Mar 17, 2021

laurenreidexpensify commented Mar 17, 2021

roryabraham commented Mar 24, 2021

parasharrajat commented Mar 24, 2021

roryabraham commented Mar 24, 2021

roryabraham commented Mar 24, 2021

roryabraham commented Mar 24, 2021

parasharrajat commented Mar 24, 2021

roryabraham commented Mar 24, 2021

marcaaron commented Mar 24, 2021

parasharrajat commented Mar 24, 2021

shawnborton commented Mar 24, 2021

parasharrajat commented Mar 24, 2021

roryabraham commented Mar 24, 2021

roryabraham commented Mar 24, 2021

laurenreidexpensify commented Mar 25, 2021

laurenreidexpensify commented Feb 17, 2021 •

edited

Loading

parasharrajat commented Feb 17, 2021 •

edited

Loading

parasharrajat commented Feb 19, 2021 •

edited

Loading

parasharrajat commented Feb 19, 2021 •

edited

Loading

marcaaron commented Feb 20, 2021 •

edited

Loading

parasharrajat commented Feb 20, 2021 •

edited

Loading

parasharrajat commented Mar 5, 2021 •

edited by roryabraham

Loading

parasharrajat commented Mar 9, 2021 •

edited

Loading

parasharrajat commented Mar 11, 2021 •

edited

Loading

roryabraham commented Mar 11, 2021 •

edited

Loading

parasharrajat commented Mar 11, 2021 •

edited

Loading