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.

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

fix: Accounting - Unnecessary tooltip showing the same icon on export page. #52436

Merged
merged 7 commits into from
Dec 10, 2024
Merged

fix: Accounting - Unnecessary tooltip showing the same icon on export page. #52436

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7caaa26
fix: Accounting - Unnecessary tooltip showing the same icon on export…
Krishna2323 Nov 12, 2024
898ef6b
Merge branch 'Expensify:main' into krishna2323/issue/51656
Krishna2323 Nov 21, 2024
bdb8268
Merge branch 'Expensify:main' into krishna2323/issue/51656
Krishna2323 Nov 26, 2024
902806b
fix: show tooltip for all type of chats and remove tooltips limit.
Krishna2323 Nov 26, 2024
a19cc11
Merge branch 'Expensify:main' into krishna2323/issue/51656
Krishna2323 Dec 6, 2024
ccb5446
remove list's tooltips limit.
Krishna2323 Dec 6, 2024
1de9712
revert changes in SidebarUtils & OptionRow.
Krishna2323 Dec 7, 2024
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: 9 additions & 1 deletion src/components/MultipleAvatars.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ function MultipleAvatars({
fallbackUserDetails={{
displayName: icons.at(0)?.name,
}}
shouldRender={shouldShowTooltip}
>
<View style={avatarContainerStyles}>
<Avatar
Expand Down Expand Up @@ -194,6 +195,7 @@ function MultipleAvatars({
fallbackUserDetails={{
displayName: icon.name,
}}
shouldRender={shouldShowTooltip}
>
<View style={[StyleUtils.getHorizontalStackedAvatarStyle(index, overlapSize), StyleUtils.getAvatarBorderRadius(size, icon.type)]}>
<Avatar
Expand Down Expand Up @@ -221,6 +223,7 @@ function MultipleAvatars({
<Tooltip
// We only want to cap tooltips to only 10 users or so since some reports have hundreds of users, causing performance to degrade.
text={tooltipTexts.slice(avatarRows.length * maxAvatarsInRow - 1, avatarRows.length * maxAvatarsInRow + 9).join(', ')}
shouldRender={shouldShowTooltip}
>
<View
style={[
Expand Down Expand Up @@ -260,6 +263,7 @@ function MultipleAvatars({
fallbackUserDetails={{
displayName: icons.at(0)?.name,
}}
shouldRender={shouldShowTooltip}
>
{/* View is necessary for tooltip to show for multiple avatars in LHN */}
<View>
Expand All @@ -282,6 +286,7 @@ function MultipleAvatars({
fallbackUserDetails={{
displayName: icons.at(1)?.name,
}}
shouldRender={shouldShowTooltip}
>
<View>
<Avatar
Expand All @@ -296,7 +301,10 @@ function MultipleAvatars({
</View>
</UserDetailsTooltip>
) : (
<Tooltip text={tooltipTexts.slice(1).join(', ')}>
<Tooltip
text={tooltipTexts.slice(1).join(', ')}
shouldRender={shouldShowTooltip}
>
<View style={[singleAvatarStyle, styles.alignItemsCenter, styles.justifyContentCenter]}>
<Text
style={[styles.userSelectNone, size === CONST.AVATAR_SIZE.SMALL ? styles.avatarInnerTextSmall : styles.avatarInnerText]}
Expand Down
4 changes: 1 addition & 3 deletions src/components/SelectionList/BaseSelectionList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,6 @@ function BaseSelectionList<TItem extends ListItem>(
const isDisabled = !!section.isDisabled || item.isDisabled;
const isItemFocused = (!isDisabled || item.isSelected) && focusedIndex === normalizedIndex;
const isItemHighlighted = !!itemsToHighlight?.has(item.keyForList ?? '');
// We only create tooltips for the first 10 users or so since some reports have hundreds of users, causing performance to degrade.
const showTooltip = shouldShowTooltips && normalizedIndex < 10;

Copy link
Member

Choose a reason for hiding this comment

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

We can't remove this as it as added as a performance improvement. There might not be visual effects in performance but it might impact rendering. Let's not worry about this here. We can open a slack thread for the same if needed. It was added in #26091.

Copy link
Member

Choose a reason for hiding this comment

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

@thiagobrez Any thoughts on why you added this?

Copy link
Contributor

Choose a reason for hiding this comment

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

@parasharrajat This was just carried over from the old List component when doing the refactor

Copy link
Member

Choose a reason for hiding this comment

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

But it looks like you added it here https://github.com/Expensify/App/pull/26091/files#diff-58e9afec1ac387c92300582afe20b46d7ae577a515cf97f5c4ba58948ad4fd46R249 @thiagobrez. In that case, could you please tell me the name of the file where it was used earlier?

Copy link
Contributor

Choose a reason for hiding this comment

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

@parasharrajat I believe it was OptionsSelector. Sorry for not being able to provide much more context, as I'm not contributing to Expensify anymore for more than a year :D

You should find more context in the tracker issue here: #11795
And here: #21048

Copy link
Member

Choose a reason for hiding this comment

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

@Krishna2323 I asked the same thing from the author(thiagobrez) above. There is no clear reason for this so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I thought that was a different PR where we did the migration. Sorry for that.

Copy link
Member

Choose a reason for hiding this comment

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

Let's create a slack thread for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a slack thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I can't provide any context as I wasn't the reviewer of that PR, @Santhosh-Sellavel was 😉

return (
<View onLayout={(event: LayoutChangeEvent) => onItemLayout(event, item?.keyForList)}>
Expand All @@ -503,7 +501,7 @@ function BaseSelectionList<TItem extends ListItem>(
index={index}
isFocused={isItemFocused}
isDisabled={isDisabled}
showTooltip={showTooltip}
showTooltip={shouldShowTooltips}
canSelectMultiple={canSelectMultiple}
onLongPressRow={onLongPressRow}
shouldSingleExecuteRowSelect={shouldSingleExecuteRowSelect}
Expand Down
2 changes: 1 addition & 1 deletion src/libs/OptionsListUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ function getHeaderMessageForNonUserList(hasSelectableOptions: boolean, searchVal
* Helper method to check whether an option can show tooltip or not
*/
function shouldOptionShowTooltip(option: ReportUtils.OptionData): boolean {
return (!option.isChatRoom || !!option.isThread) && !option.private_isArchived;
return !option.private_isArchived;
}

/**
Expand Down
5 changes: 1 addition & 4 deletions src/pages/home/HeaderView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,7 @@ function HeaderView({report, parentReportAction, reportID, onNavigationMenuButto
/>
) : (
<OfflineWithFeedback pendingAction={report?.pendingFields?.avatar}>
<MultipleAvatars
icons={icons}
shouldShowTooltip={!isChatRoom || isChatThread}
/>
<MultipleAvatars icons={icons} />
</OfflineWithFeedback>
)}
<View
Expand Down
Loading