-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Remove lingering border from AddressSearch component #6115
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
Changes from all commits
2cce114
ac9ba8f
2cd29d9
d8e24c7
7cf6954
3355b5e
b90728a
bb496a6
a686289
10746c2
bff165e
20a6050
ae573d5
a018166
40b428f
191cbbc
0db98c5
6ecacd5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import _ from 'underscore'; | ||
| import React, {useEffect, useRef} from 'react'; | ||
| import React, {useEffect, useState, useRef} from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import {LogBox} from 'react-native'; | ||
| import {GooglePlacesAutocomplete} from 'react-native-google-places-autocomplete'; | ||
|
|
@@ -35,8 +35,12 @@ const defaultProps = { | |
| containerStyles: null, | ||
| }; | ||
|
|
||
| // Do not convert to class component! It's been tried before and presents more challenges than it's worth. | ||
| // Relevant thread: https://expensify.slack.com/archives/C03TQ48KC/p1634088400387400 | ||
| // Reference: https://github.com/FaridSafi/react-native-google-places-autocomplete/issues/609#issuecomment-886133839 | ||
| const AddressSearch = (props) => { | ||
| const googlePlacesRef = useRef(); | ||
| const [displayListViewBorder, setDisplayListViewBorder] = useState(false); | ||
| useEffect(() => { | ||
| if (!googlePlacesRef.current) { | ||
| return; | ||
|
|
@@ -84,7 +88,12 @@ const AddressSearch = (props) => { | |
| fetchDetails | ||
| suppressDefaultStyles | ||
| enablePoweredByContainer={false} | ||
| onPress={(data, details) => saveLocationDetails(details)} | ||
| onPress={(data, details) => { | ||
| saveLocationDetails(details); | ||
|
|
||
| // After we select an option, we set displayListViewBorder to false to prevent UI flickering | ||
| setDisplayListViewBorder(false); | ||
| }} | ||
| query={{ | ||
| key: 'AIzaSyC4axhhXtpiS-WozJEsmlL3Kg3kXucbZus', | ||
| language: props.preferredLocale, | ||
|
|
@@ -107,14 +116,20 @@ const AddressSearch = (props) => { | |
| if (!_.isEmpty(googlePlacesRef.current.getAddressText()) && !isTextValid) { | ||
| saveLocationDetails({}); | ||
| } | ||
|
|
||
| // If the text is empty, we set displayListViewBorder to false to prevent UI flickering | ||
| if (_.isEmpty(text)) { | ||
| setDisplayListViewBorder(false); | ||
| } | ||
| }, | ||
| }} | ||
| styles={{ | ||
| textInputContainer: [styles.flexColumn], | ||
| listView: [ | ||
| styles.borderTopRounded, | ||
| styles.borderBottomRounded, | ||
| styles.mt1, | ||
| !displayListViewBorder && styles.googleListView, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By doing this, we are applying
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I used this workaround to prevent an UI flicker.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 think the reality of this situation is that we are not guaranteed that we'll ever make our own 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough 👍 My best guess for why the flicker happens is:
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 😄
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the reason we are not considering a fork and just add an extra condition in this We would just have to add something like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I have the answer to that question. Maybe @Luke9389 has more context here?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| displayListViewBorder && styles.borderTopRounded, | ||
| displayListViewBorder && styles.borderBottomRounded, | ||
| displayListViewBorder && styles.mt1, | ||
| styles.overflowAuto, | ||
| styles.borderLeft, | ||
| styles.borderRight, | ||
|
|
@@ -127,6 +142,12 @@ const AddressSearch = (props) => { | |
| description: [styles.googleSearchText], | ||
| separator: [styles.googleSearchSeparator], | ||
| }} | ||
| onLayout={(event) => { | ||
| // We use the height of the element to determine if we should hide the border of the listView dropdown | ||
| // to prevent a lingering border when there are no address suggestions. | ||
| // The height of the empty element is 2px (1px height for each top and bottom borders) | ||
| setDisplayListViewBorder(event.nativeEvent.layout.height > 2); | ||
| }} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.