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

[HOLD for mentions doc] Add "handle" next to display name in report actions #15993

Closed
puneetlath opened this issue Mar 15, 2023 · 23 comments
Closed
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@puneetlath
Copy link
Contributor

puneetlath commented Mar 15, 2023

As discussed here, we would like to add "handles" next to the display name in chat messages. It is the @amy style text that is to the right of Amy Evans name in the mockup.

Chat - Desktop (11)

The handles would be displayed with the following logic:

  • Handles are only shown when they differ from the user's display name. We don't show the handle when it's the exact same as the display name because it is redundant. This is because we use the primary login as the display name when a first/last name isn't set.

The general logic for how we want to display the handle is laid out in pseudocode here:

if (displayName == "") {
    displayName = primaryLogin;
}

if (primaryLogin on viewer's domain && primaryLogin not publicEmail) {
    handle = frontOf(primaryLogin);
} else {
    handle = primaryLogin;
}

if (displayName == handle) {
    header = "<b>displayName</b>";
} else {
    header = "<b>displayName</b> @handle";
}

In words:

  • When we are displaying a handle:
    • The handle is prefixed with an @ symbol
    • If their primary login is a phone number, we show the full phone number as the handle (e.g. @9765473456)
    • If their primary login is a public domain email address, we show the full email address as the handle (e.g. @[email protected])
    • If their primary login is a private domain email address on a different domain than the logged in user, we show the full email address as the handle (e.g. @[email protected])
    • If their primary login is a private domain email address on the same domain as the logged in user, we show the front of the email address as the handle (e.g. @bob)
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01901c3233ac120b6f
  • Upwork Job ID: 1637899128487186432
  • Last Price Increase: 2023-03-20
@puneetlath puneetlath added Weekly KSv2 NewFeature Something to build that is a new item. labels Mar 15, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 15, 2023
@puneetlath puneetlath self-assigned this Mar 15, 2023
@puneetlath
Copy link
Contributor Author

I don't think the front-end knows whether the domain is a public domain. So I'll need to have the back-end return that in the personal details.

@puneetlath
Copy link
Contributor Author

Actually, that's not true. It's in the PUBLIC_DOMAINS constant in expensify-common, which is used in this component for example.

@Expensify Expensify unlocked this conversation Mar 16, 2023
@ArekChr
Copy link
Contributor

ArekChr commented Mar 17, 2023

Hey @puneetlath I can work on a proposal for this issue

@puneetlath
Copy link
Contributor Author

Sounds great @ArekChr!

I've updated the mockups and made some slight tweaks to the described behavior based on the discussion in this thread.

@ArekChr
Copy link
Contributor

ArekChr commented Mar 20, 2023

Hey, @puneetlath, I assume this feature is only about displaying handles?
Do we want support displaying a list of users when typing @... in chat input, or will that be a separate feature?

Edit:
Also, the same question about handles that are highlighted in inside messages, I see it in the design, but this is not mentioned in the description. That be a separate feature?

@puneetlath
Copy link
Contributor Author

Hey @ArekChr that's correct, that will be a separate feature that's currently in planning. For now, we just want to display the handles in the chat header here.

Screenshot 2023-03-20 at 10 04 13 AM

@ArekChr
Copy link
Contributor

ArekChr commented Mar 20, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue.

As discussed, we must add a "handle" next to the user's display name in chat messages. The handle should only be displayed when it differs from the user's display name. We must ensure the handle is displayed correctly based on the user's primary login.
An example is the @amy style text that is to the right of Amy Evans's name in the mockup.

What changes do you think we should make in order to solve the problem?

I propose creating a reusable component called UserHandle that will contain the necessary logic to display the handle. This component will accept a user object and return the formatted user name with the handle.
I will also add tests covering all cases to ensure the handles are displayed correctly.

To summarize, I will use the following logic for displaying the handle:

  • If the user's display name is an empty string, we will use their primary login as the display name.
  • If the user's primary login is on the viewer's domain and is not a public email, we will show the front of the primary login as the handle.
  • If the user's primary login is a phone number, we will show the full phone number as the handle.
  • If the user's primary login is a public email address, we will show the full address as the handle.
  • If the user's primary login is a private domain email address on a different domain than the logged-in user, we will show the full email address as the handle.
  • If the user's primary login is a private domain email address on the same domain as the logged-in user, we will show the front of the email address as the handle.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 20, 2023
@puneetlath puneetlath added the Internal Requires API changes or must be handled by Expensify staff label Mar 20, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~01901c3233ac120b6f

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @Santhosh-Sellavel (Internal)

@puneetlath
Copy link
Contributor Author

@ArekChr given that the handle is just text, why create a component rather than just a function that returns the text for the handle? Not opposed to a component, just want to understand the reasoning.

@puneetlath puneetlath added Weekly KSv2 and removed Daily KSv2 labels Mar 20, 2023
@ArekChr
Copy link
Contributor

ArekChr commented Mar 20, 2023

@puneetlath, I assumed that this component would be designed for reuse in both the chat message body and header sections, with a component as its main building block. This would allow for different styles to be applied using a single property, such as the "variant" property, which could take values like "chat", "profile", and more. For instance, if the "chat" variant were used, the handle would have a blue background, while the "profile" variant would make it grey.

This approach offers a simpler component from the outside since styling does not need to be set externally when reusing it. Instead, it is defined once inside and can be easily reused with different styles by passing the appropriate variant as a property. For example:
<TextHandle user={user} variant="chat"/>
<TextHandle user={user} variant="profile"/>

Alternatively, I could create a function that returns the handle string, which would require passing the string inside a component and styling it externally using StyleUtils. However, this approach would require us to remember to use both the StyleUtils and the function that generates the handle each time it is used in the code.
Example:
<Text style={StyleUtilts.getStylesForHandle(user, "chat")}>{getHandleForUser(user)}<Text/>
<Text style={StyleUtilts.getStylesForHandle(user, "profile")}>{getHandleForUser(user)}<Text/>
or
<Text style={StyleUtilts.getStylesForChatHandle(user)}>{getHandleForUser(user)}<Text/>
<Text style={StyleUtilts.getStylesForProfileHandle(user)}>{getHandleForUser(user)}<Text/>

An additional advantage of the first reusable component is that it allows for separate testing of the function that generates the handle, the component that displays the specific element, and the colours for particular variants.

In both cases, the handle still needs to be passed into the text component because it will always have different styles to distinguish it from the rest of the text (in a chat message or next to the display name). That's why I assumed it could be a component which has more advantages, IMO.

What do you think about these options?

@puneetlath
Copy link
Contributor Author

Got it! I think you are right that this will be a better approach when we are using the handle in more places. However, our general approach is to go with the simplest solution for our needs now, then refactor to make it more DRY as more use-cases come up.

So I think it's better for us to go with a function that returns text for now. And then when we figure out where else we want to display the handle, we can evaluate creating a component and moving the function's logic there.

@ArekChr
Copy link
Contributor

ArekChr commented Mar 21, 2023

Sure, I will go with the general approach! Can I start working on PR?

@puneetlath
Copy link
Contributor Author

Yes, go ahead!

@puneetlath
Copy link
Contributor Author

How's this one going @ArekChr?

@ArekChr
Copy link
Contributor

ArekChr commented Mar 28, 2023

Hey, @puneetlath, I didn't get a chance to focus on the handle last week as I had other things that needed my attention. However, I am available and ready to continue working on this task.

@ArekChr ArekChr mentioned this issue Mar 29, 2023
55 tasks
@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Mar 29, 2023
@ArekChr
Copy link
Contributor

ArekChr commented Apr 4, 2023

Hey @puneetlath, I will PR is almost ready. I will do a self-review and improve the code, plus I will also include more test cases, and then I will open this PR.
Do we have some design requirements regarding truncating the display name and handle for mobile?

@gedu
Copy link
Contributor

gedu commented Apr 13, 2023

Hey, from Callstack - expert contributor group, I can continue since @ArekChr is on sick leave

@mountiny mountiny assigned gedu and unassigned ArekChr Apr 13, 2023
@puneetlath
Copy link
Contributor Author

puneetlath commented Apr 13, 2023

@gedu sounds good.

We had actually put this PR on hold and were discussing in this design doc. Would you mind responding to the comment I tagged you in?

@puneetlath puneetlath changed the title Add "handle" next to display name in report actions [HOLD for mentions doc] Add "handle" next to display name in report actions Apr 20, 2023
@puneetlath
Copy link
Contributor Author

@gedu while this issue is on hold, would you want to take this one: #18101

It was detailed in the same section of the mentions doc, but we decided to break it out separately.

@puneetlath
Copy link
Contributor Author

@gedu just wanted to check in again and see if you want to take #18101

@gedu
Copy link
Contributor

gedu commented May 9, 2023

Sorry, I think I missed the notification, we are discussing internally and will back to you if I will take it

@puneetlath
Copy link
Contributor Author

I'm going to go ahead and close this for now. I'll open a fresh issue when the design doc has been updated and we're ready to implement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants