-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 payment 2024-02-14] [HOLD for payment 2024-01-18] Consolidate GetDislplayName methods #31312
Comments
Current assignee @puneetlath is eligible for the NewFeature assigner, not assigning anyone new. |
ready to work on this issue |
@puneetlath I've analyzed the methods you mentioned and looking for another parts of code that have to do something with creating a displayName. For now leaving my findings for each method:
|
for your PR - I think we can merge it and then I will work on mine. I would rather split the whole refactor into several smaller PRs. It would be easier to test for regressions. |
Ok sounds good. I'll get it ready for review. |
@koko57 my PR was merged. |
If dedicated C+ is needed to review all sub-PRs for this issue, I am happy to help. |
@puneetlath I've opened the first draft PR. I removed the getDisplayNameForTypingIndicator method. I actually wonder if we need to check if the we have user's login or ID in reportUserIsTyping object. I tried to create a new user in OldDot and then was typing a message in NewDot it looked like we have the ID. But maybe it's for the old users? So leaving this logic now, let me know if it's still necessary. Another thing: while working with ReportTypingIndicator (the only component that used that removed method) I noticed that we don't really need this other component App/src/pages/home/report/ReportTypingIndicator.js Lines 46 to 52 in 8c9925d
App/src/pages/home/report/ReportTypingIndicator.js Lines 57 to 63 in 8c9925d
and it actually looks the same (screenshots - I changed for the red color the implementation with Text instead of TextWithEllipsis) I'm trying to find the reason why now we use two different components for displaying this text. Maybe I'm missing something? If it would be ok to use the Text only for both cases, we could remove TextWithEllipsis (it's only used in ReportTypingIndicator). What do you think? 🙂 |
@puneetlath and regarding the issue and the next steps:
|
Sounds good.
Yes, I agree with this. Do you @situchan?
This makes sense to me also.
Great! |
Don't we fall back on "someone is typing..." if we don't have the info?
Nice find, I think consolidation makes sense if it isn't needed. But maybe @situchan can take a deeper look and weigh in. |
Yes, we do, but I meant checking if the key is a user ID or email login. For newly created account in the OD we do have an ID for the user, but maybe for the old accounts we only have an email. But I think we can revisit this logic later on. For the removing TextWithEllipsis - as you asked @situchan - I'm waiting for the feedback before I will refactor this one. For moving the methods - I would do this in a separate PR - a PR only for moving these methods. |
It's intentional. If display name is very long, it should be truncated at name, not on typing... Original: But this isn't necessary when multiple users are typing because "Multiple users are typing..." text is short enough to display even in small screen width. |
@situchan great, thanks a lot for the explanation! So leaving it as it is. |
Agree as long as it doesn't cause cyclic dependencies issue
👍 |
PR opened for review #31495 |
opened a draft PR for moving the methods: #34657 |
Payment Summary
BugZero Checklist (@puneetlath)
|
Sounds good to me too. Thanks |
Great! Thanks! So the first part is opened: #34657 |
@situchan how's this going? |
review is in progress. will complete today |
What's left here? |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.37-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-02-14. 🎊 For reference, here are some details about the assignees on this issue: |
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@puneetlath I suggested to merge getDisplayNameForParticipant with getDisplayNameOrDefault. It's a big change, I'm still looking if there won't be any edge cases and we can use only one method everywhere. |
Ok, sounds good. Thanks for keeping me in the loop. |
@puneetlath After analyzing the usages of both methods I'm not convinced that we should go with merging them - they are slightly different and I would need to introduce an additional layer of complexity to cover all the cases. For now, I think that both methods serve their purpose in the places they are used and are easy to use. |
Payment Summary
BugZero Checklist (@puneetlath)
|
Agree. Thanks |
Ok sounds good. @situchan I see two "payment summary" posts in the issue. Is that correct? I feel like there were more PRs than that 😅 |
Great, thanks. Offer sent: https://www.upwork.com/nx/wm/offer/100915132 Please ping me here when you've accepted. |
Accepted thanks |
Paid. Thanks y'all! |
We have a bunch of different methods that are used to get the display name:
I would like to do two things:
PersonalDetailsUtils.getDisplayNameOrDefault
andPersonalDetails.getDisplayName
andReportUtils.getDisplayNameForParticipant
could beReportUtils.getDisplayNameForParticipant
doesn't fall back on the login, even if it is available, and I would like to update it to do soThe text was updated successfully, but these errors were encountered: