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.

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 TextLink text wrapping #6113

Merged
merged 1 commit into from
Oct 29, 2021
Merged

Fix TextLink text wrapping #6113

merged 1 commit into from
Oct 29, 2021

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Oct 29, 2021

Previously, we assumed that TextLink components behaved like Text components–i.e. if the text exceeds with the width of the container, it should wrap. However, this wasn't happening because we were using a Pressable internally for TextLink components, which was causing some strange behavior with how TextLink components were displaying when used in-line with other text.

This fixes TextLink to use Text under the hood. It gets rid of the ability for us to apply additional styles on hover/press.

Before

After

Simulator Screen Shot - iPhone 13 - 2021-10-29 at 13 26 02

Fixed Issues

$ #6078

Tests

  1. Create a file called edit_content.diff in your App directory with the following:
diff --git src/pages/EnablePayments/index.js src/pages/EnablePayments/index.js
index 8012df050..7c329f587 100644
--- src/pages/EnablePayments/index.js
+++ src/pages/EnablePayments/index.js
@@ -28,11 +28,12 @@ class EnablePaymentsPage extends React.Component {
    }

    render() {
-        if (_.isEmpty(this.props.userWallet)) {
-            return <FullScreenLoadingIndicator />;
-        }
+        // if (_.isEmpty(this.props.userWallet)) {
+        //     return <FullScreenLoadingIndicator />;
+        // }

-        const currentStep = this.props.userWallet.currentStep || CONST.WALLET.STEP.ONFIDO;
+        // const currentStep = this.props.userWallet.currentStep || CONST.WALLET.STEP.ONFIDO;
+        const currentStep = CONST.WALLET.STEP.ONFIDO;
        return (
            <ScreenWrapper>
                {currentStep === CONST.WALLET.STEP.ONFIDO && <OnfidoStep />}
diff --git src/pages/ReimbursementAccount/ReimbursementAccountPage.js src/pages/ReimbursementAccount/ReimbursementAccountPage.js
index 757fd7ab1..3e7abb986 100644
--- src/pages/ReimbursementAccount/ReimbursementAccountPage.js
+++ src/pages/ReimbursementAccount/ReimbursementAccountPage.js
@@ -159,7 +159,7 @@ class ReimbursementAccountPage extends React.Component {
        // mounts which will set the achData.currentStep after the account data is fetched and overwrite the logical
        // next step.
        const achData = lodashGet(this.props, 'reimbursementAccount.achData', {});
-        const currentStep = achData.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;
+        const currentStep = CONST.BANK_ACCOUNT.STEP.REQUESTOR || achData.currentStep || CONST.BANK_ACCOUNT.STEP.BANK_ACCOUNT;
        if (this.props.reimbursementAccount.loading) {
            const isSubmittingVerificationsData = _.contains([
                CONST.BANK_ACCOUNT.STEP.COMPANY,
diff --git src/pages/iou/IOUModal.js src/pages/iou/IOUModal.js
index 2957a4c28..59d1c99b0 100755
--- src/pages/iou/IOUModal.js
+++ src/pages/iou/IOUModal.js
@@ -250,7 +250,7 @@ class IOUModal extends Component {
        const reportID = lodashGet(this.props, 'route.params.reportID', '');

        // If the user is trying to send money, then they need to upgrade to a GOLD wallet
-        if (this.isSendRequest && !this.hasGoldWallet) {
+        if (true){//this.isSendRequest && !this.hasGoldWallet) {
            Navigation.navigate(ROUTES.IOU_ENABLE_PAYMENTS);
            return;
        }
  1. Apply this diff. This will allow us to navigate through the Send Money flow to the Onfido Flow without having to set up the test account's wallet.
patch -p0 -i edit_content.diff
  1. Sign into NewDot, click the global create menu > Send Money.
  2. Click through the steps until you're taken to a modal "Verify Identity".
  3. Verify that the text links display correctly (as in the screenshots below).
  4. Navigate to Preferences and change the language to Spanish.
  5. Repeat steps 3-5 again, verify everything displays correctly in Spanish.

QA Steps

Test Account:

  • username: janeil21@mbakingzl.com
  • password: asdfASDF00
  1. Sign into NewDot with the test account, click the global create menu > Send Money.
  2. Click through the steps until you're taken to a modal "Verify Identity".
  3. Verify that the text links display correctly (as in the screenshots below).
  4. Sign into OldDot with the test account and run the following in the console:
NVP.set('preferredLocale', 'es')
  1. Repeat steps 1-3 again, verify everything displays correctly in Spanish.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-10-29 at 1 46 15 PM

Mobile Web

Screen Shot 2021-10-29 at 1 56 48 PM

Desktop

Screen Shot 2021-10-29 at 1 47 11 PM

iOS

Simulator Screen Shot - iPhone 13 - 2021-10-29 at 13 26 02

Android

Screenshot_20211029-140120_New Expensify

@jasperhuangg jasperhuangg self-assigned this Oct 29, 2021
@jasperhuangg jasperhuangg marked this pull request as ready for review October 29, 2021 21:02
@jasperhuangg jasperhuangg requested a review from a team as a code owner October 29, 2021 21:02
@MelvinBot MelvinBot requested review from aldo-expensify and removed request for a team October 29, 2021 21:02
Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Works well for me, just one detail, left a comment.

<Text
style={[styles.link, ...additionalStyles]}
accessibilityRole="link"
href={props.href}
Copy link
Contributor

@aldo-expensify aldo-expensify Oct 29, 2021

Choose a reason for hiding this comment

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

href doesn't seem to be a prop of our Text component, I think it should be removed. We pass those props down to the react-native Text component, but that component doesn't use href either (https://reactnative.dev/docs/text#props)

Update: ignore my comment. I can see that it still have some effect. Hovering a Text with href will make it show the url in the bottom corner of the page as it ends up being used as the <a href="...">

Copy link
Contributor

@aldo-expensify aldo-expensify left a comment

Choose a reason for hiding this comment

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

Looks good to me! Tested on web and Android.

@aldo-expensify aldo-expensify merged commit 87a0897 into main Oct 29, 2021
@aldo-expensify aldo-expensify deleted the jasper-textLinkWrapping branch October 29, 2021 23:36
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @aldo-expensify in version: 1.1.11-4 🚀

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

@nazam100
Copy link

nazam100 commented Nov 2, 2021

Accessibility issue Focus is not trapped within the dialogue
WCAG Failure: 2.4.3
Expected Result: The Focus should be correctly trapped within the dialogue when navigating through keyboard strokes.
Actual Result: The Focus is not trapped within the dialogue when navigating through the keyboard.

Attachment:

2021-11-02.20-49-44.online-video-cutter.com.mp4

@OSBotify
Copy link
Contributor

OSBotify commented Nov 2, 2021

🚀 Deployed to production by @yuwenmemon in version: 1.1.12-3 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants