Skip to content

Viacheslav first names #2724

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.

Sign up for GitHub

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

Merged
pecanoro merged 6 commits into from
May 13, 2021
Merged

Viacheslav first names #2724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2b9e1c4
use firstName in HeaderView
Viacheslav80 May 6, 2021
c9f7812
Merge branch 'Expensify:main' into viacheslav-firstNames
Viacheslav80 May 6, 2021
c805330
firstName instead of displayName in sideBar
Viacheslav80 May 11, 2021
ffcca3e
remove const reportTitle + lint
Viacheslav80 May 12, 2021
b397e8a
added a check for one participant or multiple participants
Viacheslav80 May 12, 2021
b037e17
fix bug empty displayName if empty firstName and lastName
Viacheslav80 May 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/pages/home/HeaderView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,14 @@ const defaultProps = {

const HeaderView = (props) => {
const participants = lodashGet(props.report, 'participants', []);
const reportTitle = lodashGet(props.report, 'reportName', '');
const isMultipleParticipant = participants.length > 1;
const displayNamesWithTooltips = _.map(
getPersonalDetailsForLogins(participants, props.personalDetails),
({displayName, login}) => ({displayName, tooltip: login}),
({displayName, firstName, login}) => (
{displayName: (isMultipleParticipant ? firstName : displayName) || login, tooltip: login}
),
);

const fullTitle = displayNamesWithTooltips.map(({displayName}) => displayName).join(', ');
return (
<View style={[styles.appContentHeader]} nativeID="drag-area">
<View style={[styles.appContentHeaderTitle, !props.isSmallScreenWidth && styles.pl5]}>
Expand Down Expand Up @@ -94,7 +96,7 @@ const HeaderView = (props) => {
/>
<View style={[styles.flex1, styles.flexRow]}>
<DisplayNames
fullTitle={reportTitle}
fullTitle={fullTitle}
displayNamesWithTooltips={displayNamesWithTooltips}
tooltipEnabled
numberOfLines={1}
Expand Down
9 changes: 6 additions & 3 deletions src/pages/home/sidebar/OptionRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,14 @@ const OptionRow = ({
? hoverStyle.backgroundColor
: backgroundColor;
const focusedBackgroundColor = styles.sidebarLinkActive.backgroundColor;
const isMultipleParticipant = option.participantsList.length > 1;

@roryabraham roryabraham May 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line in particular caused a regression, because the option.participantsList may be undefined per the prop-types here

@Viacheslav80 Viacheslav80 May 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

const displayNamesWithTooltips = _.map(
option.participantsList,
({displayName, login}) => ({displayName, tooltip: login}),
({displayName, firstName, login}) => (
{displayName: (isMultipleParticipant ? firstName : displayName) || login, tooltip: login}
),
);

const fullTitle = displayNamesWithTooltips.map(({displayName}) => displayName).join(', ');

@marcaaron marcaaron May 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a big deal, but I think this should maybe be moved to OptionsListUtils and happen for option.text

return (
<Hoverable>
{hovered => (
Expand Down Expand Up @@ -159,7 +162,7 @@ const OptionRow = ({
}
<View style={contentContainerStyles}>
<DisplayNames
fullTitle={option.text}

@marcaaron marcaaron May 18, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Viacheslav80 Viacheslav80 May 18, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

fullTitle={fullTitle}
displayNamesWithTooltips={displayNamesWithTooltips}
tooltipEnabled={showTitleTooltip}
numberOfLines={1}
Expand Down