fix get language locale from map provider #891
Conversation
|
Looks good. I think we should probably add a little unit test coverage as well |
static/js/theme-map/Util/helpers.js
Outdated
| if (supportedLocalesForProvider.includes(localeStr)) { | ||
| return localeStr; | ||
| } | ||
| const provider = (mapProvider === 'google') ? GoogleMaps : MapboxMaps; |
There was a problem hiding this comment.
should we display an error if mapprovider is not google or mapbox? maybe we want to extract this logic to a separate helper since it's also used in ThemeMap.js?
There was a problem hiding this comment.
yeah, that would help if something is not configure right. I will update this
There was a problem hiding this comment.
do you think it would be better to default to google/mapbox, or just log error and return null?
There was a problem hiding this comment.
maybe log an error and return 'en'? I think returning null would make it blow up later down the road. If we want it to blow up I think we should just throw an error, though I imagine logging an error is good enough to not hide bugs but still handle things gracefully
There was a problem hiding this comment.
ooo sorry you mean for the helper, yeah I think that's fine!
There was a problem hiding this comment.
we probably don't want to default, I think the provider already defaults to mapbox somewhere else in the code?
There was a problem hiding this comment.
I think you are referring to the loadAndInitializeMap() in ThemeMap for defaulting to mapbox? which we would replace with our helper call. So I guess to keep the functionality the same, I would default to Mapbox.
### Bug Fixes - Fixed the logic of the `getLanguageForProvider` utility method. This method maps the `locale` specified by the user to one that is understood by the chosen map provider. (#891) - Corrected a styling issue with the `SpellCheck` query suggestion. This issue made it difficult to click the query suggestion and fire off a new query. (#892)
### Bug Fixes - Fixed the logic of the `getLanguageForProvider` utility method. This method maps the `locale` specified by the user to one that is understood by the chosen map provider. (#891) - Corrected a styling issue with the `SpellCheck` query suggestion. This issue made it difficult to click the query suggestion and fire off a new query. (#892)
getLanguageForProviderwas using an undefined variable supportedLocalesforProvider. Updated function toget provider instance based on given provider name, and check if the given locale is within the list of provider's supported locales.
J=TECHOPS-1288
TEST=manual
used client experience that uses locale en_GB and see that map is loaded on page
added jest test for getLanguageForProvider()