fix: missing copy icons and social icons on Android #20528

s-alves10 · 2023-06-09T16:00:17Z

Details

Fixed Issues

Note: This PR is a workaround to fix #17368. Current version of react-native has some bugs in Text component. numberOfLines should be removed once facebook/react-native#35703 is merged and applied to our repo

Tests

Go to login page. If you have signed in, please sign out.
Verify that 5 social icons at the bottom of the page are displayed and working fine
Login NewDot with any account
Go to Settings -> Profile -> Contact Methods
Verify that copy icon is displayed and working fine
Go to Settings -> Workspaces -> any workspace -> Reimbursements
Verify that copy icon is displayed and working fine

Verify that no errors appear in the JS console

Offline tests

Go to login page. If you have signed in, please sign out.
Verify that 5 social icons at the bottom of the page are displayed and working fine
Login NewDot with any account
Go to Settings -> Profile -> Contact Methods
Verify that copy icon is displayed and working fine
Go to Settings -> Workspaces -> any workspace -> Reimbursements
Verify that copy icon is displayed and working fine

QA Steps

Please note the bugs identified by @aimane-chnaif here and here should be categorized as new bugs and not regressions from this PR

Go to login page. If you have signed in, please sign out.
Verify that 5 social icons at the bottom of the page are displayed and working fine
Login NewDot with any account
Go to Settings -> Profile -> Contact Methods
Verify that copy icon is displayed and working fine
Go to Settings -> Workspaces -> any workspace -> Reimbursements
Verify that copy icon is displayed and working fine

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Web

17368_mac_safari.mp4

Mobile Web - Chrome

17368_android_chrome.mp4

Mobile Web - Safari

17368_ios_safari.mp4

Desktop

17368_mac_desktop.mp4

iOS

17368_ios_native.mp4

Android

17368_android_native.mp4

…android(magic numberOfLines)

melvin-bot · 2023-06-09T16:06:03Z

@NikkiWines @aimane-chnaif One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

aimane-chnaif

WorkspaceReimburseView is also using CopyTextToClipboard so update that as well.

aimane-chnaif · 2023-06-12T16:58:34Z

src/pages/signin/Socials.js

+                style={[styles.mr1, styles.mt1]}
+                shouldUseAutoHitSlop={false}


What's this change for?

mr1 is for spacing between icons. mt1 is for row spacing in cases icons are wrapped.
If shouldUseAutoHitSlop is enabled, touch area of the pressable gets larger. To prevent this, we need to disable it

I don't see any difference between with shouldUseAutoHitSlop={false} and without.
Can you explain more detail with screenshots?

Edit: nvm, I found issue on iOS. I see now why hitSlop should not be applied here

src/components/PressableWithDelayToggle.js

aimane-chnaif · 2023-06-12T17:00:05Z

src/components/PressableWithDelayToggle.js

                    >
                        {props.isDelayButtonStateComplete && props.textChecked ? props.textChecked : props.text}
+                        &nbsp;


Why did you replace mr1 with space?

mr1 in this context doesn't work. That's why I replaced it with space

ok this regression came from another PR. But that's good to fix it here as it's simple.
Also I found another 2 regressions happening on production:

copy icon is not center aligned vertically

copy icon is not highlighted even though cursor changed to pointer changes to pointer

Screen.Recording.2023-06-13.at.11.48.27.AM.mov

This can be out of scope as it doesn't seem simple solution.
cc: @NikkiWines

Yep, I think it's fine to make those separate issues but please note it in the PR so that our QA team doesn't mark them as regressions from this PR. Once this is live, they should be reported as new bugs.

src/pages/settings/Profile/Contacts/ContactMethodsPage.js

src/pages/workspace/reimburse/WorkspaceReimburseView.js

aimane-chnaif · 2023-06-13T10:53:02Z

WorkspaceReimburseView is also using CopyTextToClipboard so update that as well.

@s-alves10 did you check this?

Sorry I meant WorkspaceBillsFirstSection

s-alves10 · 2023-06-13T11:34:10Z

WorkspaceReimburseView is also using CopyTextToClipboard so update that as well.

@s-alves10 did you check this?

Fixed

aimane-chnaif · 2023-06-13T11:45:26Z

src/pages/signin/Socials.js

+                style={[styles.mr1, styles.mt1]}
+                shouldUseAutoHitSlop={false}


I don't see any difference between with shouldUseAutoHitSlop={false} and without.
Can you explain more detail with screenshots?

Edit: nvm, I found issue on iOS. I see now why hitSlop should not be applied here

src/pages/signin/Socials.js

src/pages/workspace/bills/WorkspaceBillsFirstSection.js

s-alves10 · 2023-06-13T12:39:23Z

@aimane-chnaif

I've fixed all you mentioned. Will you check it again?

aimane-chnaif · 2023-06-13T12:56:07Z

Keyboard accessibility bugs:

First Enter key doesn't open link. Instead hit Login button
After opening link in new tab from Enter key and return back, icon button is still focused and highlighted

I think this is not a blocker as we're not focusing accessibility issues for now.

keyboard.accessibility.mov

aimane-chnaif · 2023-06-13T12:58:25Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

web.mov

Mobile Web - Chrome

mchrome.mov

Mobile Web - Safari

msafari.mov

Desktop

desktop.mov

ios2.mov

iOS

ios1.mov

Android

android.mov

aimane-chnaif

#20528 (comment)

Except above issues, look good.

@NikkiWines all yours!

s-alves10 · 2023-06-13T21:04:22Z

@NikkiWines
Will you review this PR?

NikkiWines

Couple of small suggestions

NikkiWines · 2023-06-13T23:13:45Z

src/pages/workspace/reimburse/WorkspaceReimburseView.js

+                        {/* Workaround to fix https://github.com/Expensify/App/issues/17368.
+                            TODO: Remove `numberOfLines` once https://github.com/facebook/react-native/pull/35703 is merged and applied to our repo */}


I would remove these comments, they clutter up the code and we don't know when that PR is going to be merged.

Instead, I think it's better to link to facebook/react-native#35703 in the PR Details section and provide the same context there.

@s-alves10 thanks for updating the code. What I meant above is that you should add details from that comment to the PR Details section (see screenshot) so that anyone who comes back to this PR in the future knows the context of why this was modified like this and what PR we're waiting on.

NikkiWines · 2023-06-13T23:15:11Z

src/components/PressableWithDelayToggle.js

                    >
                        {props.isDelayButtonStateComplete && props.textChecked ? props.textChecked : props.text}
+                        &nbsp;


Yep, I think it's fine to make those separate issues but please note it in the PR so that our QA team doesn't mark them as regressions from this PR. Once this is live, they should be reported as new bugs.

src/pages/workspace/reimburse/WorkspaceReimburseView.js

s-alves10 · 2023-06-14T00:09:19Z

@NikkiWines

PR details updated and comments were removed.

NikkiWines

👍

NikkiWines · 2023-06-14T17:24:32Z

@s-alves10 please see this comment about updating the PR description

Applause team, please note the bugs identified by @aimane-chnaif here and here should be categorized as new bugs and not regressions from this PR 🙇

OSBotify · 2023-06-14T17:28:52Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

melvin-bot · 2023-06-14T17:33:59Z

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

melvin-bot · 2023-06-14T17:36:37Z

❗ Please, do not use Github auto-linking keywords such as these: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

For more details, see the Contributing Guidelines, specifically Submit your pull request for a final review 📖.

OSBotify · 2023-06-14T21:25:11Z

🚀 Deployed to staging by https://github.com/NikkiWines in version: 1.3.28-0 🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	failure ❌
🍎 iOS 🍎	failure ❌
🕸 web 🕸	failure ❌

OSBotify · 2023-06-16T18:40:18Z

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

sangar-1028 added 2 commits June 9, 2023 09:35

fix: copy icon in contact-methods & workspace-reimbursements page on …

917e7b6

…android(magic numberOfLines)

fix: social icons refactor

ed87945

s-alves10 marked this pull request as ready for review June 9, 2023 16:05

s-alves10 requested a review from a team as a code owner June 9, 2023 16:05

melvin-bot bot requested review from aimane-chnaif and NikkiWines and removed request for a team June 9, 2023 16:06

s-alves10 mentioned this pull request Jun 9, 2023

[HOLD for payment 2023-06-23] [$2000] Android - Copy icon is missing in Profile/Contact method #17368

Closed

6 tasks

fix: conflicts

e713d2b

aimane-chnaif suggested changes Jun 12, 2023

View reviewed changes

fix: comments to workaround

51bdf0e

fix: WorkspaceBillsFirstSectioin

26db64b

aimane-chnaif reviewed Jun 13, 2023

View reviewed changes

src/pages/workspace/bills/WorkspaceBillsFirstSection.js Outdated Show resolved Hide resolved

sangar-1028 added 2 commits June 13, 2023 07:02

fix: mistake

511db7e

fix: reduce margin of social icons(parent)

6706964

aimane-chnaif approved these changes Jun 13, 2023

View reviewed changes

NikkiWines requested changes Jun 13, 2023

View reviewed changes

fix: remove todo comments

b2d1fcb

sangar-1028 added 3 commits June 13, 2023 21:31

Merge branch 'main' into fix/issue-17368

d08df01

fix: conflicts

dec8ce3

fix: lint error

38ff87c

s-alves10 requested a review from NikkiWines June 14, 2023 02:54

NikkiWines approved these changes Jun 14, 2023

View reviewed changes

NikkiWines merged commit ee0ffa8 into Expensify:main Jun 14, 2023

OSBotify mentioned this pull request Jun 14, 2023

Deploy Checklist: New Expensify 2023-06-14 #20782

Closed

aimane-chnaif mentioned this pull request Jun 14, 2023

20601 — migrate PressableWithDelayToggle #20658

Merged

57 tasks

s-alves10 deleted the fix/issue-17368 branch November 22, 2023 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: missing copy icons and social icons on Android #20528

fix: missing copy icons and social icons on Android #20528

s-alves10 commented Jun 9, 2023 •

edited

Loading

melvin-bot bot commented Jun 9, 2023

aimane-chnaif left a comment

aimane-chnaif Jun 12, 2023

s-alves10 Jun 12, 2023

aimane-chnaif Jun 13, 2023 •

edited

Loading

aimane-chnaif Jun 12, 2023

s-alves10 Jun 12, 2023

aimane-chnaif Jun 13, 2023

NikkiWines Jun 13, 2023

s-alves10 Jun 14, 2023

aimane-chnaif commented Jun 13, 2023 •

edited

Loading

s-alves10 commented Jun 13, 2023

aimane-chnaif Jun 13, 2023 •

edited

Loading

s-alves10 commented Jun 13, 2023

aimane-chnaif commented Jun 13, 2023

aimane-chnaif commented Jun 13, 2023

aimane-chnaif left a comment

s-alves10 commented Jun 13, 2023

NikkiWines left a comment

NikkiWines Jun 13, 2023

s-alves10 Jun 14, 2023

NikkiWines Jun 14, 2023

NikkiWines Jun 13, 2023

s-alves10 commented Jun 14, 2023

NikkiWines left a comment

NikkiWines commented Jun 14, 2023

OSBotify commented Jun 14, 2023

melvin-bot bot commented Jun 14, 2023

melvin-bot bot commented Jun 14, 2023

OSBotify commented Jun 14, 2023

OSBotify commented Jun 16, 2023

		style={[styles.mr1, styles.mt1]}
		shouldUseAutoHitSlop={false}

		{/* Workaround to fix https://github.com/Expensify/App/issues/17368.
		TODO: Remove `numberOfLines` once https://github.com/facebook/react-native/pull/35703 is merged and applied to our repo */}

fix: missing copy icons and social icons on Android #20528

fix: missing copy icons and social icons on Android #20528

Conversation

s-alves10 commented Jun 9, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Jun 9, 2023

aimane-chnaif left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aimane-chnaif Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aimane-chnaif commented Jun 13, 2023 • edited Loading

s-alves10 commented Jun 13, 2023

aimane-chnaif Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

s-alves10 commented Jun 13, 2023

aimane-chnaif commented Jun 13, 2023

aimane-chnaif commented Jun 13, 2023

Reviewer Checklist

Screenshots/Videos

aimane-chnaif left a comment

Choose a reason for hiding this comment

s-alves10 commented Jun 13, 2023

NikkiWines left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s-alves10 commented Jun 14, 2023

NikkiWines left a comment

Choose a reason for hiding this comment

NikkiWines commented Jun 14, 2023

OSBotify commented Jun 14, 2023

melvin-bot bot commented Jun 14, 2023

melvin-bot bot commented Jun 14, 2023

OSBotify commented Jun 14, 2023

OSBotify commented Jun 16, 2023

s-alves10 commented Jun 9, 2023 •

edited

Loading

aimane-chnaif Jun 13, 2023 •

edited

Loading

aimane-chnaif commented Jun 13, 2023 •

edited

Loading

aimane-chnaif Jun 13, 2023 •

edited

Loading