feat(location-field): make location group order configurable #771
Conversation
| const OtherFeaturesHeader = () => ( | ||
| <S.MenuGroupHeader as={headingType} bgColor="#333" key="other-header"> | ||
| <FormattedMessage | ||
| description="Text for header above the 'other'" |
There was a problem hiding this comment.
Description looks unfinished, could you fix please?
…/otp-ui into reorder-other-to-first
|
Blocked by #774 |
amy-corson-ibigroup
added
the
BLOCKERS
amy-corson-ibigroup
removed
the
BLOCKERS
| const [otherControl, setOtherControl] = useState(false); | ||
| return ( | ||
| <> | ||
| <label htmlFor="other-input"> |
There was a problem hiding this comment.
Could you make Storybook create the controls using argTypes instead? (Storybook will add that to its "Controls" tab - I should have mentioned that in my previous review but somehow thought you knew) Do that for this story only. Refer to e.g.
amy-corson-ibigroup
requested review from
miles-grant-ibigroup
and removed request for
josh-willis-arcadis
packages/location-field/src/types.ts
Outdated
| /** | ||
| * If true, make "Other" category (addresses, POIs, etc.) first in the results container. | ||
| */ | ||
| renderOtherFirst?: boolean; |
There was a problem hiding this comment.
Can we make this an array instead? Where we can manually set an order of categories to prioritize (and those will be displayed first). This might require a bigger refactor but I think it's worth it. Let me know if you'd like to discuss in more detail offline
There was a problem hiding this comment.
so like ["STOPS", "STATIONS", "OTHER"] as the config item?
There was a problem hiding this comment.
Update in 03c02ac!!
… geocoder options
| description="Text for header above Stations" | ||
| id="otpUi.LocationField.stations" | ||
| description="Text for header above the 'other' category of geocoder results" | ||
| id={`otpUi.LocationField.${titleId}`} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
| layerColorMap={layerColorMap} | ||
| locationType="from" | ||
| onLocationSelected={onLocationSelected} | ||
| geocoderResultsOrder={["OTHER", "STATIONS", "STOPS"]} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Added in e2b352e!
| description="Text for header above the 'other' category of geocoder results" | ||
| id={`otpUi.LocationField.${titleId}`} | ||
| id={title118nId} |
There was a problem hiding this comment.
This is pretty messy. Can we just pass in JSX and have the calling function call FormattedMessage?
There was a problem hiding this comment.
Added in e2b352e, but let me know if this is not what you meant!
Allows configuration of the order of the returned location group results, so that if you pass
renderOtherFirstthrough the config, it puts the "other" category first in the results, above stops and stations.