Skip to content

fix: use tooltip on money request header avatar #19947

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

Closed
allroundexperts wants to merge 14 commits into from
Closed

fix: use tooltip on money request header avatar #19947

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a993d50
fix: use tooltip on money request header avatar
allroundexperts Jun 1, 2023
d49c710
Merge branch 'main' into fix-19341
allroundexperts Jun 6, 2023
7edf9b7
feat: show emails instead of displayname on tooltip for MoneyRequestH…
allroundexperts Jun 6, 2023
09dc45d
fix: make money request header consistent with chats
allroundexperts Jun 6, 2023
85d7662
feat: use full title on avatar with display names for expense report …
allroundexperts Jun 14, 2023
c67aeaf
Merge branch 'main' into fix-19341
allroundexperts Jun 14, 2023
41249bf
Merge branch 'main' into fix-19341
allroundexperts Jun 19, 2023
b0ed114
feat: use new tooltips
allroundexperts Jun 19, 2023
e977d63
feat: remove tooltip for workspace text
allroundexperts Jun 19, 2023
a2e1681
feat: lint errors
allroundexperts Jun 19, 2023
ecfab30
Merge branch 'main' into fix-19341
allroundexperts Jun 20, 2023
79ec0e2
fix: in-consistency between web and mobile for display name
allroundexperts Jun 24, 2023
dcf2db5
fix: failing unit tests
allroundexperts Jun 24, 2023
4f146c7
fix: failing unit tests
allroundexperts Jun 24, 2023
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
21 changes: 13 additions & 8 deletions src/components/AvatarWithDisplayName.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import reportPropTypes from '../pages/reportPropTypes';
import participantPropTypes from './participantPropTypes';
import withWindowDimensions, {windowDimensionsPropTypes} from './withWindowDimensions';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import UserDetailsTooltip from './UserDetailsTooltip';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
import SubscriptAvatar from './SubscriptAvatar';
Expand Down Expand Up @@ -71,13 +72,17 @@ function AvatarWithDisplayName(props) {
size={props.size}
/>
) : (
<Avatar
size={props.size}
source={icons[0].source}
type={icons[0].type}
name={icons[0].name}
containerStyles={avatarContainerStyle}
/>
<UserDetailsTooltip accountID={props.report.ownerAccountID}>
<View>
<Avatar
size={props.size}
source={icons[0].source}
type={icons[0].type}
name={icons[0].name}
containerStyles={avatarContainerStyle}
/>
</View>
</UserDetailsTooltip>
)}
<View style={[styles.flex1, styles.flexColumn, styles.ml3]}>
<DisplayNames
Expand All @@ -86,7 +91,7 @@ function AvatarWithDisplayName(props) {
tooltipEnabled
numberOfLines={1}
textStyles={[props.isAnonymous ? styles.headerAnonymousFooter : styles.headerText, styles.pre]}
shouldUseFullTitle={isExpenseReport || props.isAnonymous}
shouldUseFullTitle={props.isAnonymous}

@sobitneupane sobitneupane Jun 7, 2023

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 change also doesn't look related to this PR. Change commented above and this change will result in following change in UI.

Before:
Screenshot 2023-06-07 at 13 03 53

After:
Screenshot 2023-06-07 at 13 04 05

I don't mind the change in UI as it looks consistent with other headers. But I am not sure why was it created that way by @yuwenmemon in #18148. May be some design specification.

@yuwenmemon Do you think it is okay to make this change here?
cc: @youssef-lr

@allroundexperts allroundexperts Jun 7, 2023

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.

@sobitneupane Exactly. I did this to make it more consistent.

@allroundexperts allroundexperts Jun 8, 2023 •
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.

@youssef-lr @yuwenmemon This is blocked on your reply here

@youssef-lr youssef-lr Jun 14, 2023

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.

I think it's intentional to only display the full name based on those two conditions so I'd say let's remove that change from this PR.

@allroundexperts allroundexperts Jun 14, 2023

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.

@sobitneupane Removed.

@allroundexperts allroundexperts Jun 21, 2023

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.

@sobitneupane How so? Can you share screenshots of web vs native?

@sobitneupane sobitneupane Jun 21, 2023

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.

Screenshot 2023-06-21 at 18 14 01 Screenshot 2023-06-21 at 18 14 32

@sobitneupane sobitneupane Jun 24, 2023

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.

bump @allroundexperts

@allroundexperts allroundexperts Jun 24, 2023

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.

Fixed @sobitneupane!

@sobitneupane sobitneupane Jun 26, 2023 •
edited
Loading

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.

I think it's intentional to only display the full name based on those two conditions so I'd say let's remove that change from this PR.

@allroundexperts As suggested by @youssef-lr I think the better idea is to remove this change from this PR. I cannot get how this change is related to the issue we are trying to solve.

Or, if we want to make this change in this PR. Let's first get approval from the team. Started a discussion here.

/>
{!_.isEmpty(subtitle) && (
<Text
Expand Down
14 changes: 11 additions & 3 deletions src/components/DisplayNames/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,24 @@
import React from 'react';
import React, {Fragment} from 'react';
import _ from 'underscore';
import {propTypes, defaultProps} from './displayNamesPropTypes';
import Text from '../Text';
import styles from '../../styles/styles';

// As we don't have to show tooltips of the Native platform so we simply render the full display names list.
function DisplayNames(props) {
return (
<Text
accessibilityLabel={props.accessibilityLabel}
style={props.textStyles}
numberOfLines={props.numberOfLines}
>
{props.fullTitle}
{props.shouldUseFullTitle
? props.fullTitle
: _.map(props.displayNamesWithTooltips, ({displayName}, index) => (
<Fragment key={index}>
<Text style={[...props.textStyles, styles.pre]}>{displayName}</Text>
{index < props.displayNamesWithTooltips.length - 1 && <Text style={props.textStyles}>,&nbsp;</Text>}
</Fragment>
))}
</Text>
);
}
Expand Down
39 changes: 27 additions & 12 deletions src/components/MoneyRequestHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {View} from 'react-native';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import HeaderWithBackButton from './HeaderWithBackButton';
import UserDetailsTooltip from './UserDetailsTooltip';
import iouReportPropTypes from '../pages/iouReportPropTypes';
import withLocalize, {withLocalizePropTypes} from './withLocalize';
import * as ReportUtils from '../libs/ReportUtils';
Expand Down Expand Up @@ -115,19 +116,33 @@ function MoneyRequestHeader(props) {
<Text style={[styles.textLabelSupporting, styles.lh16]}>{props.translate('common.to')}</Text>
<View style={[styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween, styles.pv3]}>
<View style={[styles.flex1, styles.flexRow, styles.alignItemsCenter, styles.justifyContentBetween]}>
<Avatar
source={payeeAvatar}
type={isExpenseReport ? CONST.ICON_TYPE_WORKSPACE : CONST.ICON_TYPE_AVATAR}
name={payeeName}
size={CONST.AVATAR_SIZE.DEFAULT}
/>
<UserDetailsTooltip
accountID={isExpenseReport ? '' : moneyRequestReport.managerID}
fallbackUserDetails={{
avatar: payeeAvatar,
displayName: ReportUtils.getPolicyName(moneyRequestReport, props.policies),
}}
>
<View>
<Avatar
source={payeeAvatar}
type={isExpenseReport ? CONST.ICON_TYPE_WORKSPACE : CONST.ICON_TYPE_AVATAR}
name={payeeName}
size={CONST.AVATAR_SIZE.DEFAULT}
/>
</View>
</UserDetailsTooltip>
<View style={[styles.flex1, styles.flexColumn, styles.ml3]}>
<Text
style={[styles.headerText, styles.pre]}
numberOfLines={1}
>
{payeeName}
</Text>
<View style={[styles.alignItemsStart]}>
<UserDetailsTooltip accountID={isExpenseReport ? '' : moneyRequestReport.managerID}>
<Text
style={[styles.headerText, styles.pre]}
numberOfLines={1}
>
{payeeName}
</Text>
</UserDetailsTooltip>
</View>
{isExpenseReport && (
<Text
style={[styles.textLabelSupporting, styles.lh16, styles.pre]}
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/UnreadIndicatorsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,11 +357,11 @@ describe('Unread Indicators', () => {
expect(displayNameTexts).toHaveLength(2);
const firstReportOption = displayNameTexts[0];
expect(lodashGet(firstReportOption, ['props', 'style', 0, 'fontWeight'])).toBe(fontWeightBold);
expect(lodashGet(firstReportOption, ['props', 'children'])).toBe('C User');
expect(firstReportOption).toHaveTextContent('C User');

const secondReportOption = displayNameTexts[1];
expect(lodashGet(secondReportOption, ['props', 'style', 0, 'fontWeight'])).toBe(fontWeightBold);
expect(lodashGet(secondReportOption, ['props', 'children'])).toBe('B User');
expect(secondReportOption).toHaveTextContent('B User');

// Tap the new report option and navigate back to the sidebar again via the back button
return navigateToSidebarOption(0);
Expand All @@ -372,9 +372,9 @@ describe('Unread Indicators', () => {
const displayNameTexts = screen.queryAllByLabelText(hintText);
expect(displayNameTexts).toHaveLength(2);
expect(lodashGet(displayNameTexts[0], ['props', 'style', 0, 'fontWeight'])).toBe(undefined);
expect(lodashGet(displayNameTexts[0], ['props', 'children'])).toBe('C User');
expect(displayNameTexts[0]).toHaveTextContent('C User');
expect(lodashGet(displayNameTexts[1], ['props', 'style', 0, 'fontWeight'])).toBe(fontWeightBold);
expect(lodashGet(displayNameTexts[1], ['props', 'children'])).toBe('B User');
expect(displayNameTexts[1]).toHaveTextContent('B User');
}));

xit('Manually marking a chat message as unread shows the new line indicator and updates the LHN', () =>
Expand Down
13 changes: 6 additions & 7 deletions tests/unit/SidebarFilterTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {cleanup, screen} from '@testing-library/react-native';
import Onyx from 'react-native-onyx';
import lodashGet from 'lodash/get';
import * as LHNTestUtils from '../utils/LHNTestUtils';
import waitForPromisesToResolve from '../utils/waitForPromisesToResolve';
import CONST from '../../src/CONST';
Expand Down Expand Up @@ -331,7 +330,7 @@ describe('Sidebar', () => {
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1);
expect(displayNames).toHaveLength(1);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
expect(displayNames[0]).toHaveTextContent('Three, Four');
} else {
// Both reports visible
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down Expand Up @@ -371,8 +370,8 @@ describe('Sidebar', () => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(2);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('One, Two');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('Three, Four');
expect(displayNames[0]).toHaveTextContent('One, Two');
expect(displayNames[1]).toHaveTextContent('Three, Four');
})

// When report3 becomes unread
Expand Down Expand Up @@ -440,8 +439,8 @@ describe('Sidebar', () => {
const hintText = Localize.translateLocal('accessibilityHints.chatUserDisplayNames');
const displayNames = screen.queryAllByLabelText(hintText);
expect(displayNames).toHaveLength(2);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
expect(lodashGet(displayNames, [1, 'props', 'children'])).toBe('One, Two');
expect(displayNames[0]).toHaveTextContent('Three, Four');
expect(displayNames[1]).toHaveTextContent('One, Two');
})
);
});
Expand Down Expand Up @@ -652,7 +651,7 @@ describe('Sidebar', () => {
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
expect(screen.queryAllByAccessibilityHint(navigatesToChatHintText)).toHaveLength(1);
expect(displayNames).toHaveLength(1);
expect(lodashGet(displayNames, [0, 'props', 'children'])).toBe('Three, Four');
expect(displayNames[0]).toHaveTextContent('Three, Four');
} else {
// Both reports visible
const navigatesToChatHintText = Localize.translateLocal('accessibilityHints.navigatesToChat');
Expand Down
Loading