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 all 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
32 changes: 32 additions & 0 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,16 @@ 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),
});
}
return;
}

Expand Down Expand Up @@ -250,6 +281,7 @@ 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>
Expand Down
12 changes: 12 additions & 0 deletions src/libs/actions/Transaction.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ import ONYXKEYS from '../../ONYXKEYS';
import * as CollectionUtils from '../CollectionUtils';
import * as API from '../API';

let recentWaypoints = [];
Onyx.connect({
key: ONYXKEYS.NVP_RECENT_WAYPOINTS,
callback: (val) => (recentWaypoints = val || []),
});

const allTransactions = {};
Onyx.connect({
key: ONYXKEYS.COLLECTION.TRANSACTION,
Expand Down Expand Up @@ -87,6 +93,12 @@ function saveWaypoint(transactionID, index, waypoint) {
},
},
});
const recentWaypointAlreadyExists = _.find(recentWaypoints, (recentWaypoint) => recentWaypoint.address === waypoint.address);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI this caused a deploy blocker

if (!recentWaypointAlreadyExists) {
const clonedWaypoints = _.clone(recentWaypoints);
clonedWaypoints.unshift(waypoint);
Onyx.merge(ONYXKEYS.NVP_RECENT_WAYPOINTS, clonedWaypoints.slice(0, 5));
Comment on lines +98 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of doing slice 0, 4 first and add the other item. but not a blocker

}
}

function removeWaypoint(transactionID, currentIndex) {
Expand Down
21 changes: 20 additions & 1 deletion src/pages/iou/WaypointEditor.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React, {useRef} from 'react';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import {View} from 'react-native';
import PropTypes from 'prop-types';
Expand Down Expand Up @@ -53,7 +54,7 @@ const defaultProps = {
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 +152,7 @@ function WaypointEditor({transactionID, route: {params: {iouType = '', waypointI
lng: null,
state: null,
}}
predefinedPlaces={recentWaypoints}
/>
</View>
</Form>
Expand All @@ -169,5 +171,22 @@ export default compose(
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.slice(0, 5) : [], (waypoint) => ({
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,
};