Viacheslav first names #2724
Viacheslav first names #2724
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
@puneetlath Did we want to change the first names only in the chat title or also the left navigation menu under "Chats"? |
|
In both. |
|
ok. I'll do it in both cases |
|
Updated |
|
Awesome! Could you update the tests and the screenshots as well, please? |
|
Updated |
|
I fix bug empty displayName if empty firstName and lastName. if this happens we show login |
|
Updated |
|
🚀 Deployed to staging in version: 1.0.43-2🚀
|
|
🚀 Deployed to production in version: 1.0.44-0🚀
|
|
It looks like this PR most likely caused a regression |
| @@ -110,11 +110,14 @@ const OptionRow = ({ | |||
| ? hoverStyle.backgroundColor | |||
| : backgroundColor; | |||
| const focusedBackgroundColor = styles.sidebarLinkActive.backgroundColor; | |||
| const isMultipleParticipant = option.participantsList.length > 1; | |||
There was a problem hiding this comment.
This line in particular caused a regression, because the option.participantsList may be undefined per the prop-types here
There was a problem hiding this comment.
Can i move part of the code from OptionRow in OptionsListUtils.js ?
98 const tooltipText = getReportParticipantsTitle(lodashGet(report, ['participants'], []));
+ const displayNamesWithTooltips = _.map(
personalDetailList,
({displayname, firstname, login}) => (
{displayname: (hasMultipleParticipants ? firstname : displayname) || login, tooltip: login}
),
);
+ const fullTitle = displayNamesWithTooltips.map(({displayName}) => displayName).join(', ');
- return {
text: report ? report.reportName : personalDetail.displayName,
+ return {
displayNamesWithTooltips,
text: hasMultipleParticipants ? fullTitle : personalDetail.displayName,
There was a problem hiding this comment.
I think we left some opportunity to clean up this code on the table.
If we aren't going to use the reportName anymore then we should see if we can stop generating it here:
and we also do it here (no idea why we aren't reusing the logic):
and we use it in the option it here (now unused it would seem - though I am not 100% sure):
@Viacheslav80 is this something you can look into?
| @@ -159,7 +162,7 @@ const OptionRow = ({ | |||
| } | |||
| <View style={contentContainerStyles}> | |||
| <DisplayNames | |||
| fullTitle={option.text} | |||
There was a problem hiding this comment.
Hmm is the option.text being used or not? Did we stop using it and then not remove it from the option object? I'm confused. This component is meant to be fairly generic so I'm not sure this is the correct place to implement the logic we've done.
There was a problem hiding this comment.
I can move part of the code from OptionRow in OptionsListUtils.js ?
98 const tooltipText = getReportParticipantsTitle(lodashGet(report, ['participants'], []));
+ const displayNamesWithTooltips = _.map(
personalDetailList,
({displayname, firstname, login}) => (
{displayname: (hasMultipleParticipants ? firstname : displayname) || login, tooltip: login}
),
);
+ const fullTitle = displayNamesWithTooltips.map(({displayName}) => displayName).join(', ');
- return {
text: report ? report.reportName : personalDetail.displayName,
+ return {
displayNamesWithTooltips,
text: hasMultipleParticipants ? fullTitle : personalDetail.displayName,
| ); | ||
|
|
||
| const fullTitle = displayNamesWithTooltips.map(({displayName}) => displayName).join(', '); |
There was a problem hiding this comment.
Not a big deal, but I think this should maybe be moved to OptionsListUtils and happen for option.text
|
@puneetlath to clarify, did we also want this change to extend to
|
|
Hi, I think this PR introduced this regression. Here's the line that throws an error. |
|
It looks like this PR has caused a separate regression: 'Name is not visible in the IOU request page' |
|
Hi @Viacheslav80. I just want to make sure that you saw the regressions that were caused by your PR. You'll need to address those in order for us to pay you out for this PR. Thanks! |
Hi! PR with fix has already been created: #3027 |
|
Ok great! |
Details
show first names instead of fullNames in the Header for group, if has't firstName showed login
Fixed Issues
Fixes #2664
Tests
QA Steps
Same as above
Tested
Screenshots
Web
Mobile Web
Desktop
iOS
Android