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

feat(location-field): make location group order configurable #771

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 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
101 changes: 66 additions & 35 deletions packages/location-field/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ const LocationField = ({
GeocodedOptionIconComponent = GeocodedOptionIcon,
geocoderConfig,
getCurrentPosition,
geocoderResultsOrder = ["STATIONS", "STOPS", "OTHER"],
hideExistingValue = false,
initialSearchResults = null,
inputPlaceholder = null,
Expand Down Expand Up @@ -644,46 +645,76 @@ const LocationField = ({
const transitFeaturesPresent =
stopFeatures.length > 0 || stationFeatures.length > 0;

// Iterate through the geocoder results
menuItems = menuItems.concat(
stationFeatures.length > 0 && (
<S.MenuGroupHeader
as={headingType}
bgColor={layerColorMap.stations}
key="gtfs-stations-header"
>
<FormattedMessage
description="Text for header above Stations"
id="otpUi.LocationField.stations"
/>
</S.MenuGroupHeader>
),
stationFeatures.map(feature => renderFeature(itemIndex++, feature)),

stopFeatures.length > 0 && (
<S.MenuGroupHeader
as={headingType}
bgColor={layerColorMap.stops}
key="gtfs-stops-header"
>
const FeaturesElements = ({
bgColor,
key,
titleId,
featuresArray
}: {
bgColor: string;
key: string;
titleId: string;
featuresArray: JSX.Element[];
}) => {
const Header = () => (
<S.MenuGroupHeader as={headingType} bgColor={bgColor} key={key}>
<FormattedMessage
description="Text for header above Stops"
id="otpUi.LocationField.stops"
description="Text for header above the 'other' category of geocoder results"
id={`otpUi.LocationField.${titleId}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this breaks the i18n string checks? Also we've spent a lot of effort and time elsewhere to not do this, are we ok with doing it here?

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 there's a way to skip any ids we're dynamically generating in the i18n check, but it doesn't feel worth it here so I passed the entire i18n id instead!

/>
</S.MenuGroupHeader>
),
stopFeatures.map(feature => renderFeature(itemIndex++, feature)),
);
return (
<>
{/* Only include the header if there are features to show */}
{titleId === "other" ? (
<Header />
) : (
transitFeaturesPresent && <Header />
amy-corson-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
)}
{featuresArray.map(feature => renderFeature(itemIndex++, feature))}
</>
);
};

transitFeaturesPresent && otherFeatures.length > 0 && (
<S.MenuGroupHeader as={headingType} bgColor="#333" key="other-header">
<FormattedMessage
description="Text for header above the 'other'"
id="otpUi.LocationField.other"
// Create an array of results to display based on the geocoderResultsOrder
const featuresElementsArray = geocoderResultsOrder.map(result => {
let Element;
if (result === "OTHER") {
amy-corson-ibigroup marked this conversation as resolved.
Show resolved Hide resolved
Element = (
<FeaturesElements
bgColor="#333"
key="other-header"
featuresArray={otherFeatures}
titleId="other"
/>
</S.MenuGroupHeader>
),
otherFeatures.map(feature => renderFeature(itemIndex++, feature))
);
);
}
if (result === "STATIONS") {
Element = (
<FeaturesElements
bgColor={layerColorMap.stations}
key="gtfs-stations-header"
featuresArray={stationFeatures}
titleId="stations"
/>
);
}
if (result === "STOPS") {
Element = (
<FeaturesElements
bgColor={layerColorMap.stops}
key="gtfs-stops-header"
featuresArray={stopFeatures}
titleId="stops"
/>
);
}
return Element;
});

// Iterate through the geocoder results
menuItems = menuItems.concat(featuresElementsArray);
}

/* 2) Process nearby transit stop options */
Expand Down
19 changes: 19 additions & 0 deletions packages/location-field/src/stories/Desktop.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -251,3 +251,22 @@ export const WithUserSettings = (): JSX.Element => (
userLocationsAndRecentPlaces={userLocationsAndRecentPlaces}
/>
);

export const WithCustomResultsOrder = (): JSX.Element => (
<LocationField
currentPosition={currentPosition}
geocoderConfig={geocoderConfig}
getCurrentPosition={getCurrentPosition}
preferredLayers={["example_layer"]}
initialSearchResults={mockedGeocoderResponse.features}
inputPlaceholder="Select from location"
layerColorMap={layerColorMap}
locationType="from"
onLocationSelected={onLocationSelected}
geocoderResultsOrder={["OTHER", "STATIONS", "STOPS"]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bit of a nit, but could we put the strings into an object so they're more consistent? Similar to how we do the view ids in otp-rr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you clarify where in OTP-RR to look for this? Also, we're using a lot of array methods here, what's the benefit of making this an object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lib/actions/ui-constants.js!

Really this should be an enum but we can't do that in JS so an object with keys that are strings is the cleanest way to do it. Using an enum is good practice, prevents silly mistakes, and prevents us from having to copy paste strings aroudn

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in e2b352e!

sortByDistance
style={{ fontFamily: "sans-serif" }}
/>
);

WithCustomResultsOrder.parameters = a11yOverrideParameters;
Loading
Loading