[$500] Android - LHN - Magic glass icon overlaps text

[lanitochka17]

Version Number: 1.4.68-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: expensify.testrail.io/index.php?/cases/view/2990351
Issue reported by: Applause - Internal Team

Action Performed:
Precondition: User is logged in Android mobile application. Preferences - #focus

Unpin all chats in LHN
Mark all chats in LHN as read
Expected Result:
"All caught up.." UI is displayed. No visual issues with text

Actual Result:
"All caught up.." UI is displayed. Magic glass icon is overlaps the text annotation

Workaround:
Unknown

Platforms:
Which of our officially supported platforms is this issue occurring on?

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop
Screenshots/Videos
Add any screenshot/video evidence

Bug6466750_1714490725789!1714490435348

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @Santhosh-Sellavel

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@alexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[ShridharGoel]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Android - LHN - Magic glass icon overlaps text.

What is the root cause of that problem?

Text rendering can vary between platforms.

https://www.codeclouds.com/blog/why-web-fonts-matter-a-short-history-and-future-of-font-rendering/

facebook/react-native#32384

On Android, having text with center alignment doesn't center align the inline icons.

What changes do you think we should make in order to solve the problem?

Add this in styles/index.ts:

emptyLHNDetailsContainer: {
    flexDirection: 'row',
    alignItems: 'center',
    justifyContent: 'center',
    flexWrap: 'wrap',
    textAlign: 'center',
}

Update emptyLHNSubtitle to use the below:

const emptyLHNSubtitle = useMemo(
    () => (
        <View style={[styles.emptyLHNDetailsContainer]}>
            <Text
                style={[
                    styles.textAlignCenter,
                    { color: theme.placeholderText },
                ]}
            >
                {translate('common.emptyLHN.subtitleText1')}
            </Text>
            <Icon
                src={Expensicons.MagnifyingGlass}
                width={variables.emptyLHNIconWidth}
                height={variables.emptyLHNIconHeight}
                fill={theme.icon}
                small
                inline
                additionalStyles={styles.mh1}
            />
            <Text
                style={[
                    styles.textAlignCenter,
                    { color: theme.placeholderText },
                ]}
            >
                {translate('common.emptyLHN.subtitleText2')}
            </Text>
            <Icon
                src={Expensicons.Plus}
                width={variables.emptyLHNIconWidth}
                height={variables.emptyLHNIconHeight}
                fill={theme.icon}
                small
                inline
                additionalStyles={styles.mh1}
            />
            <Text
                style={[
                    styles.textAlignCenter,
                    { color: theme.placeholderText },
                ]}
            >
                {translate('common.emptyLHN.subtitleText3')}
            </Text>
        </View>
    ),
    [theme, translate],
);

What alternate options did you explore?

Option 1:

Update the below to use only text and no inline icons. We'll update the text based on this comment and icons would no longer be there.

App/src/components/LHNOptionsList/LHNOptionsList.tsx

Lines 70 to 102 in ff2b440

	const emptyLHNSubtitle = useMemo(
	() => (
	<View>
	<Text
	color={theme.placeholderText}
	style={[styles.textAlignCenter]}
	>
	{translate('common.emptyLHN.subtitleText1')}
	<Icon
	src={Expensicons.MagnifyingGlass}
	width={variables.emptyLHNIconWidth}
	height={variables.emptyLHNIconHeight}
	small
	inline
	fill={theme.icon}
	additionalStyles={styles.alignItemsCenter}
	/>
	{translate('common.emptyLHN.subtitleText2')}
	<Icon
	src={Expensicons.Plus}
	width={variables.emptyLHNIconWidth}
	height={variables.emptyLHNIconHeight}
	small
	inline
	fill={theme.icon}
	additionalStyles={styles.alignItemsCenter}
	/>
	{translate('common.emptyLHN.subtitleText3')}
	</Text>
	</View>
	),
	[theme, styles.alignItemsCenter, styles.textAlignCenter, translate],
	);

Option 2:

We can remove textAlignCenter style for Android, and let the text be at the left side.

We'll create a new style called textAlignCenterWithInlineIcons which will have a Android specific implementation in index.android.ts and common implementation will be in index.ts. We can have it inside a new package in styles/utils.

In index.ts:

textAlignCenterWithInlineIcons: {
    textAlign: 'center',
}

In index.android.ts:

textAlignCenterWithInlineIcons: {
}

[alexpensify]

I can replicate the experience, but I'm not sure if this issue is device-specific because my + icon is overlapping with the copy.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01de95687e3aa21366

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel (External)

[alexpensify]

@Santhosh-Sellavel - when you get a chance, can you please review if this proposal will fix the issue? Thanks!

[Santhosh-Sellavel]

AFAIK We can use Icon inline in other places as well. So waiting for other proposals.

[ShridharGoel]

@Santhosh-Sellavel My proposal doesn't change the functionality of inline icons at other places.

[alexpensify]

Heads up, I will be offline until Tuesday, May 7, 2024, and will not actively watch over this GitHub during that period.

If anything urgent is needed here, please ask for help in the #expensify-open-source Slack Room-- thanks!

[Santhosh-Sellavel]

@Santhosh-Sellavel My proposal doesn't change the functionality of inline icons at other places.

Yes, I am aware of that. Let's wait for other proposals.

[melvin-bot]

@alexpensify, @Santhosh-Sellavel Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Santhosh-Sellavel]

Still waiting for other proposals.

[melvin-bot]

@alexpensify, @Santhosh-Sellavel Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ShridharGoel]

When we use center-alignment in Android, the text gets aligned but inline icons don't. So, I still think the way to solve this is to align text at left only in this case for Android.

@alexpensify Can we get thoughts from internal engineer and design team?

[ShridharGoel]

Proposal

Updated

[melvin-bot]

@alexpensify, @mallenexpensify, @Santhosh-Sellavel, @dubielzyk-expensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[mallenexpensify]

@Santhosh-Sellavel 👀 plz on the updated proposal above from @ShridharGoel . Thx

[Santhosh-Sellavel]

Can you reassign this @mallenexpensify, while I'm away.

cc: @Expensify/contributor-plus Any volunteers please take, thanks!

[allroundexperts]

I can take over!

[parasharrajat]

I can help.

[parasharrajat]

@ShridharGoel Could you please explain the code Suggestion you posted and tell us about your changes?

Also, how does it look on iPhone 14 pro and iPhone 8?

we can not for sure use the android specific code so that is out of the picture.

[ShridharGoel]

iPhone 8

iPhone 14

[ShridharGoel]

Could you please explain the code Suggestion you posted and tell us about your changes?

The updated solution uses flexbox to ensure proper alignment and spacing.

[parasharrajat]

Ok, I think we can go with @ShridharGoel's #41349 (comment). Their main solution looks good to me.

Keeping the same icons mix string and align it well.

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @iwiznia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[parasharrajat]

@iwiznia we also need to swap C+ as per #41349 (comment)

[iwiznia]

Assigned @ShridharGoel. Not sure how to trigger a new reassignment for C+ though... @mallenexpensify or @alexpensify do you know?

[melvin-bot]

@iwiznia @alexpensify @mallenexpensify @ShridharGoel @Santhosh-Sellavel @dubielzyk-expensify this issue is now 4 weeks old, please consider:

• Finding a contributor to fix the bug
• Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
• If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

[mallenexpensify]

Removed @Santhosh-Sellavel and assigned @parasharrajat . Should be all set.

[parasharrajat]

@ShridharGoel Feel free to move ahead with the PR as you are assigned. Offer can be sent later.

[alexpensify]

Thanks, @mallenexpensify, for the help here. I'm back online and taking over again as the BZ member.

[parasharrajat]

@ShridharGoel When can we expect the PR?

[ShridharGoel]

@parasharrajat #42801