Remove lingering border from AddressSearch component #6115

luacmartins · 2021-10-29T19:47:18Z

Details

Not my favorite solution, but it's a temporary workaround to compensate for the limitations of the library.
Wrapped the AddressSearch component in a View to track it's hide and dynamically hide the lingering border.

cc @aldo-expensify @Luke9389

Fixed Issues

$ #5810

Tests

On NewDot, navigate to /bank-account/company
Type in an address that does not exist, e.g. asdasdas
Verify that there is no lingering border below the input
Now, type in an address that exists and select it from the dropdown.
Click on the input again and verify that there is no lingering border.

QA Steps

Steps above.

Tested On

Screenshots

Web

web.mov

Mobile Web

The component is failing to connect to www.expensify.com.dev on the simulator and returns no address results. We might be missing some config here. This is happening on the main branch as well.

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

aldo-expensify

I must say that I'm a bit amazed with the clever way you found to solve this. The solution works. In other context I would have thought that this solution is too much of a workaround, but in this case I haven't seen any other solution (besides forking the library 🤮 ).

NAB 1: I feel like display state could have a better name.
NAB 2: Just for consistency, I think we stick to class components when we have states (style guide). Maybe it should be changed to a class component.

Tested on web/android both were working 🎉 🎉

src/components/AddressSearch.js

luacmartins · 2021-10-29T20:43:40Z

Updated!

luacmartins · 2021-10-29T21:04:29Z

Found some issues with the solution. Converting to draft!

luacmartins · 2021-10-29T21:16:32Z

Apparently we might have to leave this one as a functional component to make use of setAddressText - FaridSafi/react-native-google-places-autocomplete#609 (comment)

luacmartins · 2021-10-29T21:28:18Z

Updated!

aldo-expensify · 2021-10-29T21:37:27Z

Apparently we might have to leave this one as a functional component to make use of setAddressText - FaridSafi/react-native-google-places-autocomplete#609 (comment)

maybe we should leave comment in the code, so we don't accidentally convert it to class component

aldo-expensify · 2021-10-29T21:42:00Z

It is still breaking if the scroll bar appears because the error text wraps:

aldo-expensify

Tested on web and Android. It is working now including when there are error message (wrapping and not wrapping)

Luke9389

I know this is a tough issue. I've wrestled with it a lot myself. My suggestion could be hard to implement and don't break your back trying. Just give it a shot and see if it's feasible. We can discuss more.

src/components/AddressSearch.js

Luke9389 · 2021-10-30T00:25:58Z

src/components/AddressSearch.js

+        // The height of the input + wrapped error message is 112 pixels
+        <View
+            onLayout={(event) => {
+                const {height} = event.nativeEvent.layout;
+                return height > 112 ? setDisplayListViewBorder(true) : setDisplayListViewBorder(false);


I don't know the answer to this question but it seems worth asking. Can we use a more dynamic way to measure the height? We have a ref to the <GooglePlacesAutoComplete> component so maybe we can use the measure function (mentioned here).

Hmm we can definitely get the input's height dynamically. I'm not sure if it's worth it though, because we still have to manually compensate for the top margin and border width, so the conditional becomes something like this height > inputHeight + 6. Also, the solution is far from elegant 🤣

I'll spend a bit more time and see if I can come up with something better.

Actually managed to move the onLayout prop to the GooglePlacesAutocomplete component and now the solution is independent of the text input height. Thanks for the tip @Luke9389!

Cool, I'll review again

Oh rad, nice work @luacmartins! I really appreciate you following through on that. 🙇

luacmartins · 2021-11-01T17:01:49Z

Updated!

aldo-expensify

Left a comment about style guidelines.

There is a few milliseconds flickering of the border when it opens in Android, not sure if it is worth trying to fix it. When we get the results, there is no rounded top border, and then it appears.

screen-20211101-114111_2.mp4

src/components/AddressSearch.js

luacmartins · 2021-11-01T18:53:31Z

Updated!

luacmartins · 2021-11-01T19:16:46Z

I noticed the flickering as well, but in all platforms. It seems like a delay between state update and component re-rendering. I put up a workaround to hide the element for those few milliseconds - it's not a pretty solution 😞

I would imagine that we will redo a few of these inputs as part of creating our new form component, and I would suggest that we create our own version of the AddressSearch component.

Luke9389 · 2021-11-01T22:11:01Z

src/components/AddressSearch.js

-                    styles.borderTopRounded,
-                    styles.borderBottomRounded,
-                    styles.mt1,
+                    !displayListViewBorder && styles.googleListView,


By doing this, we are applying transform: {[scale:0]} whenever displayListViewBorder is false correct? Is this what you were referring to here?

I put up a workaround to hide the element for those few milliseconds - it's not a pretty solution 😞

Yes! I used this workaround to prevent an UI flicker.

Ok, gotcha. I know this has been a debugging marathon, but I think we should try and get to the bottom of why the top border flicker is happening.

I would imagine that we will redo a few of these inputs as part of creating our new form component, and I would suggest that we create our own version of the AddressSearch component.

I think the reality of this situation is that we are not guaranteed that we'll ever make our own AddressSearch component. And if we can fix it here and make it work perfectly for us, we'll never have to. I know it feels like we're wrestling with jankyness we didn't create (which is true) but we're so close to finally polishing off this component.

I'd be happy to help with debugging this. I won't have time to do that tonight, but I can check out this branch and poke around tomorrow if you'd like another pair of eyes on this.

Fair enough 👍 My best guess for why the flicker happens is:

User starts typing and we fetch results. The height of the dropdown changes and it becomes visible.

We detect the height change, set displayListViewBorder=true and apply the styles.

So I think that the component becomes visible before we can apply any style changes. This might be a limitation of my solution.

I explored other ways of preventing the flicker, but couldn't come up with anything. Another set of eyes are always appreciated. Feel free to take a look 😄

What is the reason we are not considering a fork and just add an extra condition in this if statement:

https://github.com/FaridSafi/react-native-google-places-autocomplete/blob/47d7223dd48f85da97e80a0729a985bbbcee353f/GooglePlacesAutocomplete.js#L751-L757

We would just have to add something like: dataSource.length &&. It work right away and would save a bunch of time spent on workarounds.

I'm not sure I have the answer to that question. Maybe @Luke9389 has more context here?

So far, we haven't been faced with a situation that requires a fork, but I'd support that if it were proposed. Would either of you like to spearhead that? I'm thinking it would probably come in the form of a Problem Solution statement.

luacmartins · 2021-11-08T17:36:50Z

Ok, given the discussing around this issue, should we:

Merge this PR as a temporary fix, maybe with the hardcoded value until we hear back from the library maintainer or fork the library.
Close this PR.

Curious to hear what people think.

Luke9389 · 2021-11-08T19:10:22Z

Yea, I'd be fine with that.

MariaHCD

Having this as a temporary fix makes sense to me!

aldo-expensify

Lgtm (as temporary fix :P)

OSBotify · 2021-11-09T19:48:56Z

✋ 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-15T21:21:27Z

🚀 Deployed to staging by @aldo-expensify in version: 1.1.14-5 🚀

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

ogumen · 2021-11-21T23:02:33Z

The accessibility issues found in this PR:
Web

The company address search suggestions are not keyboard accessible - failure of WCAG SC 2.1.1
https://user-images.githubusercontent.com/88733897/142782288-c1df84e5-b934-48eb-8809-0c60f9e604d3.mp4
The presence and number of search suggestions is not conveyed by screen reader - failure of WCAG SC 4.1.3
https://user-images.githubusercontent.com/88733897/142782382-315c01e1-b998-4694-8ccc-c1845bcd9024.mp4

Android

The presence and number of search suggestions is not conveyed by screen reader - failure of WCAG SC 4.1.3

OSBotify · 2021-11-22T20:10:36Z

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

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

luacmartins added 3 commits October 29, 2021 12:51

track view height

2cce114

remove char

ac9ba8f

fix style

2cd29d9

luacmartins self-assigned this Oct 29, 2021

luacmartins marked this pull request as ready for review October 29, 2021 19:52

luacmartins requested a review from a team as a code owner October 29, 2021 19:52

MelvinBot requested review from timszot and removed request for a team October 29, 2021 19:52

aldo-expensify previously approved these changes Oct 29, 2021

View reviewed changes

src/components/AddressSearch.js Outdated Show resolved Hide resolved

luacmartins added 2 commits October 29, 2021 14:35

convert to class component

d8e24c7

fix style

7cf6954

luacmartins dismissed aldo-expensify’s stale review via 7cf6954 October 29, 2021 20:37

rename state variable

3355b5e

luacmartins added 2 commits October 29, 2021 14:54

componentDidMount to componentDidUpdate

b90728a

revert change

bb496a6

luacmartins removed the request for review from timszot October 29, 2021 21:03

luacmartins marked this pull request as draft October 29, 2021 21:04

luacmartins added 3 commits October 29, 2021 15:17

convert to functional component

a686289

adjust height

10746c2

fix style

bff165e

luacmartins marked this pull request as ready for review October 29, 2021 21:27

luacmartins requested a review from a team October 29, 2021 21:27

MelvinBot requested review from MariaHCD and removed request for a team October 29, 2021 21:27

remove line

a018166

aldo-expensify previously approved these changes Oct 29, 2021

View reviewed changes

Luke9389 reviewed Oct 30, 2021

View reviewed changes

move height tracking to google component

40b428f

luacmartins dismissed aldo-expensify’s stale review via 40b428f November 1, 2021 16:59

luacmartins requested review from Luke9389 and aldo-expensify November 1, 2021 17:03

aldo-expensify requested changes Nov 1, 2021

View reviewed changes

src/components/AddressSearch.js Outdated Show resolved Hide resolved

remove destructuring

191cbbc

luacmartins requested a review from aldo-expensify November 1, 2021 18:53

remove flickering

0db98c5

fix style

6ecacd5

Luke9389 reviewed Nov 1, 2021

View reviewed changes

Luke9389 approved these changes Nov 8, 2021

View reviewed changes

MariaHCD approved these changes Nov 9, 2021

View reviewed changes

aldo-expensify approved these changes Nov 9, 2021

View reviewed changes

aldo-expensify merged commit 7b2e671 into main Nov 9, 2021

aldo-expensify deleted the cmartins-fixLingeringBorder branch November 9, 2021 19:48

OSBotify mentioned this pull request Nov 15, 2021

Deploy Checklist: New Expensify 2021-11-15 #6305

Closed

70 tasks

aldo-expensify mentioned this pull request Nov 22, 2021

[HOLD for payment 2022-02-01] Pressing "Tab" key while the result list of AddressSearch is open leaves it open even after clicking outside #6235

Closed

eVoloshchak mentioned this pull request May 9, 2023

Enable Form inputs to manipulate related field values #9207

Merged

89 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove lingering border from AddressSearch component #6115

Remove lingering border from AddressSearch component #6115

luacmartins commented Oct 29, 2021 •

edited

Loading

aldo-expensify left a comment •

edited

Loading

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

aldo-expensify commented Oct 29, 2021

aldo-expensify commented Oct 29, 2021

aldo-expensify left a comment

Luke9389 left a comment

Luke9389 Oct 30, 2021

luacmartins Oct 30, 2021 •

edited

Loading

luacmartins Nov 1, 2021

aldo-expensify Nov 1, 2021

Luke9389 Nov 1, 2021

luacmartins commented Nov 1, 2021

aldo-expensify left a comment •

edited

Loading

luacmartins commented Nov 1, 2021

luacmartins commented Nov 1, 2021 •

edited

Loading

Luke9389 Nov 1, 2021

luacmartins Nov 1, 2021

Luke9389 Nov 1, 2021

luacmartins Nov 1, 2021 •

edited

Loading

aldo-expensify Nov 1, 2021

luacmartins Nov 1, 2021

Luke9389 Nov 2, 2021

luacmartins commented Nov 8, 2021 •

edited

Loading

Luke9389 commented Nov 8, 2021

MariaHCD left a comment

aldo-expensify left a comment

OSBotify commented Nov 9, 2021

OSBotify commented Nov 15, 2021

ogumen commented Nov 21, 2021 •

edited

Loading

OSBotify commented Nov 22, 2021

Remove lingering border from AddressSearch component #6115

Remove lingering border from AddressSearch component #6115

Conversation

luacmartins commented Oct 29, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

aldo-expensify left a comment • edited Loading

Choose a reason for hiding this comment

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

luacmartins commented Oct 29, 2021

aldo-expensify commented Oct 29, 2021

aldo-expensify commented Oct 29, 2021

aldo-expensify left a comment

Choose a reason for hiding this comment

Luke9389 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Oct 30, 2021 • 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

luacmartins commented Nov 1, 2021

aldo-expensify left a comment • edited Loading

Choose a reason for hiding this comment

luacmartins commented Nov 1, 2021

luacmartins commented Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luacmartins Nov 1, 2021 • 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

luacmartins commented Nov 8, 2021 • edited Loading

Luke9389 commented Nov 8, 2021

MariaHCD left a comment

Choose a reason for hiding this comment

aldo-expensify left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 9, 2021

OSBotify commented Nov 15, 2021

ogumen commented Nov 21, 2021 • edited Loading

OSBotify commented Nov 22, 2021

luacmartins commented Oct 29, 2021 •

edited

Loading

aldo-expensify left a comment •

edited

Loading

luacmartins Oct 30, 2021 •

edited

Loading

aldo-expensify left a comment •

edited

Loading

luacmartins commented Nov 1, 2021 •

edited

Loading

luacmartins Nov 1, 2021 •

edited

Loading

luacmartins commented Nov 8, 2021 •

edited

Loading

ogumen commented Nov 21, 2021 •

edited

Loading