Fix URL rendering #6392

parasharrajat · 2021-11-22T17:54:31Z

Details

Used Text node instead of Pressable to create inline layout inside the text to block overflow.
Due to on web Text does support LongPress, used display:inline to create inline block.

Fixed Issues

$ #5572
$ #4911

Tests | QA Steps

Send a new message with a link.
e.g. hello https://expensify.slack.com/archives/C01GTK53T8Q/p1632613507072000?
Check the message on native devices, does not overflow.
Resize the browser on the Web, it should wrap correctly.

Tested On

Screenshots

Web | Desktop

output_file.mp4

Mobile Web

output_file.mp4

iOS

Android

output_file.mp4

… wrapping

mountiny

Skimmed over this quickly. Looks good I just found one typo. I will test it out a bit more later.

mountiny · 2021-11-22T18:02:45Z

src/components/PressableWithSecondaryInteraction/pressableWithSecondaryInteractionPropTypes.js

@@ -21,13 +21,22 @@ const propTypes = {

    /** Prevent the default ContextMenu on web/Desktop */
    preventDefaultContentMenu: PropTypes.bool,
+
+    /** Use Text instead of Presable to create inline layout.


NAB: Typo

Suggested change

/** Use Text instead of Presable to create inline layout.

/** Use Text instead of Pressable to create inline layout.

parasharrajat · 2021-11-22T18:06:50Z

A few points for the reviewer:

I have to use styling instead of the same Text node for the web. As RN-web decided to not implement the LongPress on Text as RN.
Left unsupported onPressIn and onPressOut events as they do not have any effect but are needed for non-inline mode.
Added a detailed comment for an inline prop but it is half correct. Web does support those mentioned props as it is still using Pressable.

There is one known issue to me apart from the ones which I am fixing.

on the Web, the Mini menu does not hide when URL ContextMenu is opened. It would require a hacky solution to be fixed.

my other approach to fix this issue also fixes it but that is very complex. I still think not a priority issue so we should adopt the simpler approach over the complex one for now which PR is doing.

parasharrajat · 2021-11-22T18:07:26Z

I will add videos later before lets go over review.

parasharrajat · 2021-11-24T14:20:32Z

@mountiny Thoughts...?

mountiny · 2021-11-24T20:12:06Z

@parasharrajat Sorry, I got a bit stuck with university coursework. Looking into it now.

Tested it and seems to work well, which is great. The concern from the OG comment was the inability to set the custom delay for long press which is no really a problem since we use default and it is probably good to use default as that is what users are most likely used to.

on the Web, the Mini menu does not hide when URL ContextMenu is opened. It would require a hacky solution to be fixed.

Could you share screenshare of how this problem looks like? It does not look like a deal breaker for sure.

I like this approach so feel free to update the PR body appropriately with all QA steps for both issues and videos, I will give the code another look.

mountiny

LGTM, I feel like if the limitations do not break any current functionality and fix the listed problems, it is a good way to go. Let me know when this is ready for final look.

parasharrajat · 2021-11-24T20:18:03Z

Could you share screenshare of how this problem looks like? It does not look like a deal breaker for sure.

Correct not a deal-breaker. I was referring to this Mini-action menu on right. It hides when ContextMenu is open but not when the URL ContextMenu is..

I will update the videos and details

mountiny · 2021-11-24T20:21:32Z

@parasharrajat That is no problem:)

parasharrajat · 2021-11-24T20:33:48Z

@mountiny Changes done. Ready for final review.

mountiny

LGTM, just requesting comment to be added to two places. Thank you for the changes.

mountiny · 2021-11-24T21:48:44Z

src/components/PressableWithSecondaryInteraction/index.ios.js

-    </Pressable>
-);
+const PressableWithSecondaryInteraction = (props) => {
+    const Node = props.inline ? RNText : Pressable;


@parasharrajat I think it might be worth adding a little comment explaining this option here as well. I know you have added the comment to the PropType, but one compact line in here would be handy. Can you please add one? Thank you 🙌

mountiny · 2021-11-24T21:48:58Z

src/components/PressableWithSecondaryInteraction/index.android.js

-    </Pressable>
-);
+const PressableWithSecondaryInteraction = (props) => {
+    const Node = props.inline ? RNText : Pressable;


parasharrajat · 2021-11-24T22:03:31Z

Done

mountiny · 2021-11-24T22:51:02Z

Yikes, there is some linter problem 😬

parasharrajat · 2021-11-25T10:27:48Z

Linting issues fixed @mountiny.

OSBotify · 2021-11-25T14:23:53Z

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

OSBotify · 2021-11-29T19:57:04Z

🚀 Deployed to staging by @mountiny in version: 1.1.16-11 🚀

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

ogumen · 2021-12-01T16:36:25Z

Accessibility issues found in this PR:

The link sent as part of the message cannot be activated using keyboard - failure of WCAG SC 2.1.1 (The issue can be fixed by removing the <div tabindex="0"> wrapping around the hyperlink).

Chat_link.sent.cannot.be.activated.using.keyboard.mp4

The blue link text against white background fails minimum color contrast requirements of 4.50:1 for small text. The font is 11pt, wont weight 400. The blue (#0184FF) on white background (#FFFFFF) has a color contrast ratio of 3.65:1 - failure of WCAG SC 1.4.3

OSBotify · 2021-12-06T23:25:46Z

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

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

parasharrajat added 3 commits November 22, 2021 22:55

use Text instead of Pressable to fix the UI overflow issue and enable…

7b72e06

… wrapping

Remove unnecessary longpressdelay for Web.

029d09f

LongPress fix for web.

b5a7c53

parasharrajat requested a review from a team as a code owner November 22, 2021 17:54

MelvinBot requested review from mountiny and removed request for a team November 22, 2021 17:54

mountiny requested changes Nov 22, 2021

View reviewed changes

mountiny reviewed Nov 24, 2021

View reviewed changes

Merge branch 'main' of github.com:Expensify-app

86d6815

typo

78d8e43

mountiny self-requested a review November 24, 2021 21:45

mountiny reviewed Nov 24, 2021

View reviewed changes

parasharrajat added 2 commits November 25, 2021 03:30

Pull latest changes from main

b9f8e47

Comments

b5ed67b

Linting issues

5ce1b2c

mountiny approved these changes Nov 25, 2021

View reviewed changes

mountiny merged commit 5249e8c into Expensify:main Nov 25, 2021

OSBotify mentioned this pull request Nov 29, 2021

Deploy Checklist: New Expensify 2021-11-29 #6505

Closed

69 tasks

ogumen mentioned this pull request Dec 1, 2021

Prevent cannot read property of undefined for href.startsWith #6456

Merged

5 tasks

This was referenced Dec 6, 2021

[HOLD for payment 2021-12-13] Improve message display on smaller screen sizes #5572

Closed

[HOLD for payment 2021-12-13] URL link is mangled up with the rest of the text #4911

Closed

eVoloshchak mentioned this pull request Apr 28, 2022

[HOLD for payment 2022-05-20][$1000] Feature Request: Show link when hover over hyperlink #7844

Closed

parasharrajat deleted the urlRendering branch November 20, 2023 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix URL rendering #6392

Fix URL rendering #6392

parasharrajat commented Nov 22, 2021 •

edited

Loading

mountiny left a comment

mountiny Nov 22, 2021

parasharrajat commented Nov 22, 2021 •

edited

Loading

parasharrajat commented Nov 22, 2021

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

mountiny left a comment

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

parasharrajat commented Nov 24, 2021

mountiny left a comment

mountiny Nov 24, 2021

mountiny Nov 24, 2021

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

parasharrajat commented Nov 25, 2021

OSBotify commented Nov 25, 2021

OSBotify commented Nov 29, 2021

ogumen commented Dec 1, 2021

OSBotify commented Dec 6, 2021

	/** Use Text instead of Presable to create inline layout.
	/** Use Text instead of Pressable to create inline layout.

Fix URL rendering #6392

Fix URL rendering #6392

Conversation

parasharrajat commented Nov 22, 2021 • edited Loading

Details

Fixed Issues

Tests | QA Steps

Tested On

Screenshots

Web | Desktop

Mobile Web

iOS

Android

mountiny left a comment

Choose a reason for hiding this comment

mountiny Nov 22, 2021

Choose a reason for hiding this comment

parasharrajat commented Nov 22, 2021 • edited Loading

parasharrajat commented Nov 22, 2021

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

mountiny left a comment

Choose a reason for hiding this comment

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

parasharrajat commented Nov 24, 2021

mountiny left a comment

Choose a reason for hiding this comment

mountiny Nov 24, 2021

Choose a reason for hiding this comment

mountiny Nov 24, 2021

Choose a reason for hiding this comment

parasharrajat commented Nov 24, 2021

mountiny commented Nov 24, 2021

parasharrajat commented Nov 25, 2021

OSBotify commented Nov 25, 2021

OSBotify commented Nov 29, 2021

ogumen commented Dec 1, 2021

OSBotify commented Dec 6, 2021

parasharrajat commented Nov 22, 2021 •

edited

Loading

parasharrajat commented Nov 22, 2021 •

edited

Loading