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.

Sign up for GitHub

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

Jump to bottom

Remove lingering border from AddressSearch component #6115

Merged
merged 18 commits into from
Nov 9, 2021
Merged

Remove lingering border from AddressSearch component #6115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2cce114
track view height
luacmartins Oct 29, 2021
ac9ba8f
remove char
luacmartins Oct 29, 2021
2cd29d9
fix style
luacmartins Oct 29, 2021
d8e24c7
convert to class component
luacmartins Oct 29, 2021
7cf6954
fix style
luacmartins Oct 29, 2021
3355b5e
rename state variable
luacmartins Oct 29, 2021
b90728a
componentDidMount to componentDidUpdate
luacmartins Oct 29, 2021
bb496a6
revert change
luacmartins Oct 29, 2021
a686289
convert to functional component
luacmartins Oct 29, 2021
10746c2
adjust height
luacmartins Oct 29, 2021
bff165e
fix style
luacmartins Oct 29, 2021
20a6050
add comment and adjust height;
luacmartins Oct 29, 2021
ae573d5
update comment
luacmartins Oct 29, 2021
a018166
remove line
luacmartins Oct 29, 2021
40b428f
move height tracking to google component
luacmartins Nov 1, 2021
191cbbc
remove destructuring
luacmartins Nov 1, 2021
0db98c5
remove flickering
luacmartins Nov 1, 2021
6ecacd5
fix style
luacmartins Nov 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions src/components/AddressSearch.js
View file
Open in desktop
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';
Expand Down Expand Up @@ -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
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
const AddressSearch = (props) => {
const googlePlacesRef = useRef();
const [displayListViewBorder, setDisplayListViewBorder] = useState(false);
useEffect(() => {
if (!googlePlacesRef.current) {
return;
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

@luacmartins luacmartins Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. User starts typing and we fetch results. The height of the dropdown changes and it becomes visible.
  2. 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 😄

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand All @@ -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);
}}
/>
);
};
Expand Down
4 changes: 4 additions & 0 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2108,6 +2108,10 @@ const styles = {
fontFamily: fontFamily.GTA,
flex: 1,
},

googleListView: {
transform: [{scale: 0}],
},
};

const baseCodeTagStyles = {
Expand Down