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

Show recent waypoints when selecting address for distance request #25974

Merged
merged 8 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
4 changes: 4 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ const ONYXKEYS = {
/** The NVP with the last payment method used per policy */
NVP_LAST_PAYMENT_METHOD: 'nvp_lastPaymentMethod',

/** This NVP holds to most recent waypoints that a person has used when creating a distance request */
NVP_RECENT_WAYPOINTS: 'expensify_recentWaypoints',

/** Does this user have push notifications enabled for this device? */
PUSH_NOTIFICATIONS_ENABLED: 'pushNotificationsEnabled',

Expand Down Expand Up @@ -315,6 +318,7 @@ type OnyxValues = {
[ONYXKEYS.NVP_BLOCKED_FROM_CONCIERGE]: OnyxTypes.BlockedFromConcierge;
[ONYXKEYS.NVP_PRIVATE_PUSH_NOTIFICATION_ID]: string;
[ONYXKEYS.NVP_LAST_PAYMENT_METHOD]: Record<string, string>;
[ONYXKEYS.NVP_RECENT_WAYPOINTS]: OnyxTypes.RecentWaypoints[];
[ONYXKEYS.PUSH_NOTIFICATIONS_ENABLED]: boolean;
[ONYXKEYS.PLAID_DATA]: OnyxTypes.PlaidData;
[ONYXKEYS.IS_PLAID_DISABLED]: boolean;
Expand Down
45 changes: 39 additions & 6 deletions src/components/AddressSearch/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,26 @@ const propTypes = {
/** Should address search be limited to results in the USA */
isLimitedToUSA: PropTypes.bool,

/** A list of predefined places that can be shown when the user isn't searching for something */
predefinedPlaces: PropTypes.arrayOf(
PropTypes.shape({
/** A description of the location (usually the address) */
description: PropTypes.string,

/** Data required by the google auto complete plugin to know where to put the markers on the map */
geometry: PropTypes.shape({
/** Data about the location */
location: PropTypes.shape({
/** Lattitude of the location */
lat: PropTypes.number,

/** Longitude of the location */
lng: PropTypes.number,
}),
}),
}),
),
Comment on lines +64 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB. For consistency between inputs and outputs, I think we should use the waypoint shape here: address, lat and `lng. The GooglePlacesAutocomplete is used internally, the props shape conversion should be done inside and not outside. i.e. the Onyx selector should not change the shape of the object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I understand your point. This is why I did not go that route (but I did consider it).

  • The <AddressSearch> component is used in many places.
  • Adding logic to reformat this data inside of <AddressSearch> would only be used for ONE of the places the component is implemented and won't be used for any of the other places. This is a waste.
  • Adding logic to reformat this data inside of <AddressSearch> makes it very highly coupled to the parent component (because it is assuming knowledge about the data being passed into it). This makes the component harder to reuse
  • Making the predefinedPlaces prop with this shape allows any other component to use the same feature and not have to worry about conforming to the "waypoint shape".


/** A map of inputID key names */
renamedInputKeys: PropTypes.shape({
street: PropTypes.string,
Expand Down Expand Up @@ -102,6 +122,7 @@ const defaultProps = {
lng: 'addressLng',
},
maxInputLength: undefined,
predefinedPlaces: [],
};

// Do not convert to class component! It's been tried before and presents more challenges than it's worth.
Expand All @@ -122,6 +143,19 @@ function AddressSearch(props) {
const saveLocationDetails = (autocompleteData, details) => {
const addressComponents = details.address_components;
if (!addressComponents) {
// When there are details, but no address_components, this indicates that some predefined options have been passed
// to this component which don't match the usual properties coming from auto-complete. In that case, only a limited
// amount of data massaging needs to happen for what the parent expects to get from this function.
if (_.size(details)) {
props.onPress({
address: lodashGet(details, 'description', ''),
lat: lodashGet(details, 'geometry.location.lat', 0),
lng: lodashGet(details, 'geometry.location.lng', 0),
});

// After we select an option, we set displayListViewBorder to false to prevent UI flickering
setDisplayListViewBorder(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be outside the if condition. I think we should actually revert the change related to this line. It's better to just call this function on the onPress callback once rather then calling it before every return statement in saveLocationDetails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can move it back outside. It made more sense to move it inside before I added this block and had to repeat calling setDisplayListViewBorder in two locations.

}
return;
}

Expand Down Expand Up @@ -223,6 +257,9 @@ function AddressSearch(props) {
}

props.onPress(values);

// After we select an option, we set displayListViewBorder to false to prevent UI flickering
setDisplayListViewBorder(false);
};

return (
Expand Down Expand Up @@ -250,17 +287,13 @@ function AddressSearch(props) {
fetchDetails
suppressDefaultStyles
enablePoweredByContainer={false}
predefinedPlaces={props.predefinedPlaces}
ListEmptyComponent={
props.network.isOffline ? null : (
<Text style={[styles.textLabel, styles.colorMuted, styles.pv4, styles.ph3, styles.overflowAuto]}>{props.translate('common.noResultsFound')}</Text>
)
}
onPress={(data, details) => {
saveLocationDetails(data, details);

// After we select an option, we set displayListViewBorder to false to prevent UI flickering
setDisplayListViewBorder(false);
}}
onPress={saveLocationDetails}
query={query}
requestUrl={{
useOnPlatform: 'all',
Expand Down
20 changes: 19 additions & 1 deletion src/pages/iou/WaypointEditor.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
transaction: {},
};

function WaypointEditor({transactionID, route: {params: {iouType = '', waypointIndex = ''} = {}} = {}, network, translate, transaction}) {
function WaypointEditor({transactionID, route: {params: {iouType = '', waypointIndex = ''} = {}} = {}, network, translate, transaction, recentWaypoints}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We did not define recentWaypoints in the props above. This caused the lint check to fail. @s77rt @tgolen

Copy link
Contributor

Choose a reason for hiding this comment

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

wonder why this wasn't caught during PR CI checks

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. It got caught in our PR but not here 🤔 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

this is handled by @allroundexperts now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, yes... I'm surprised I missed it and doubly surprised that the linter missed it! Thanks for handling this.

const textInput = useRef(null);
const currentWaypoint = lodashGet(transaction, `comment.waypoints.waypoint${waypointIndex}`, {});
const waypointAddress = lodashGet(currentWaypoint, 'address', '');
Expand Down Expand Up @@ -151,6 +151,7 @@
lng: null,
state: null,
}}
predefinedPlaces={recentWaypoints}
/>
</View>
</Form>
Expand All @@ -169,5 +170,22 @@
key: (props) => `${ONYXKEYS.COLLECTION.TRANSACTION}${props.transactionID}`,
selector: (transaction) => (transaction ? {transactionID: transaction.transactionID, comment: {waypoints: lodashGet(transaction, 'comment.waypoints')}} : null),
},

recentWaypoints: {
key: ONYXKEYS.NVP_RECENT_WAYPOINTS,

// Only grab the most recent 5 waypoints because that's all that is shown in the UI. This also puts them into the format of data
// that the google autocomplete component expects for it's "predefined places" feature.
selector: (waypoints) =>
_.map(waypoints && waypoints.length ? waypoints.slice(0, 5) : [], (waypoint) => ({

Check failure on line 180 in src/pages/iou/WaypointEditor.js

View workflow job for this annotation

GitHub Actions / lint

'_' is not defined
Copy link
Contributor

Choose a reason for hiding this comment

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

Lint. Underscore is not imported

Copy link
Contributor

Choose a reason for hiding this comment

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

NAB. Just curious why we check for `waypoints.length?

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 think I was just being paranoid about calling .slice() on something that wasn't an array. It shouldn't ever be something other than an array, but you never know... I can remove it if you want.

description: waypoint.address,
geometry: {
location: {
lat: waypoint.lat,
lng: waypoint.lng,
},
},
})),
},
}),
)(WaypointEditor);
12 changes: 12 additions & 0 deletions src/types/onyx/RecentWaypoints.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
type RecentWaypoints = {
/** The full address of the waypoint */
address: string;

/** The lattitude of the waypoint */
lat: number;

/** The longitude of the waypoint */
lng: number;
};

export default RecentWaypoints;
4 changes: 2 additions & 2 deletions src/types/onyx/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import ReimbursementAccountDraft from './ReimbursementAccountDraft';
import WalletTransfer from './WalletTransfer';
import ReceiptModal from './ReceiptModal';
import MapboxAccessToken from './MapboxAccessToken';

import Download from './Download';
import PolicyMember from './PolicyMember';
import Policy from './Policy';
Expand All @@ -41,8 +40,8 @@ import ReportAction from './ReportAction';
import ReportActionReactions from './ReportActionReactions';
import SecurityGroup from './SecurityGroup';
import Transaction from './Transaction';

import Form, {AddDebitCardForm} from './Form';
import RecentWaypoints from './RecentWaypoints';

export type {
Account,
Expand Down Expand Up @@ -89,4 +88,5 @@ export type {
Transaction,
Form,
AddDebitCardForm,
RecentWaypoints,
};
Loading