Add Geolocation component #336
Conversation
| import { YextIcon } from '../icons/YextIcon'; | ||
|
|
||
| /** | ||
| * The CSS class interface for the Geolocation component. |
There was a problem hiding this comment.
I cannot use {@link Geolocation} here due to an issue with API Extractor adding _2 suffix to duplicate interface/class names. In search-ui-react, Geolocation from Typescript's "lib.dom.d.ts" file conflicts with Geolocation component.
See: microsoft/rushstack#2534 and microsoft/rushstack#2976
Open PR: microsoft/rushstack#2608
It does make it a little odd in the doc but the signature of the component is correct. What to know if the team have any thoughts on this. A work around, which is a little hacky, is to add a script to manually update the files generated from API extractor.
There was a problem hiding this comment.
I'm fine with it, the api extractor docs aren't made to be extremely beginner friendly
There was a problem hiding this comment.
could we link to Geolocation_2? or would that not resolve correctly?
There was a problem hiding this comment.
it would link correctly in the doc pages but we would see this in search-ui-react.api.md file:
There was a problem hiding this comment.
oh ok, I'm fine leaving it without a link
yen-tt
force-pushed
the
dev/geolocation
branch
from
94280c6
to
146d4f0
Compare
|
is there something up with the code coverage github action? it's not passing but haven't investigated why |
code coverage has been failing for the past couple PRs, I believe there's an issue with the visual-coverage script killing the process before it can run the rest of the commands for generating lcov coverage and moving files to the expected directory. That will be fix in another item/PR. |
yen-tt
force-pushed
the
dev/geolocation
branch
from
23d9df7
to
500f29f
Compare
| */ | ||
| radius?: number, | ||
| /** Custom handler function to call after user's position is successfully determined. */ | ||
| handleUserPosition?: (position: GeolocationPosition) => void |
There was a problem hiding this comment.
Small nit: I'm not wild about the names handleUserPosition or setIsFetchingLocation. I think the latter is a bit long and convoluted. The former is a bit vague: handleUserPosition doesn't sound like a callback that fires when the user's position is obtained.
There was a problem hiding this comment.
setIsFetchingLocation is following the common pattern -- setIsBlah -- we use for a boolean setter function of react useState (e.g. setIsFeedbackProvided or setIsOptionsDisabled) so I thought the naming here would make things consistent. Outside of the pattern, maybe setIsUserLocationFound or updateGetLocationStatus but I don't think it conveys that it can be set to a boolean true, regardless if the location was determined successfully or not, as long as the fetching process is done? Similarly with our callbacks handleBlah pattern. Let me know if you have other names you prefer!
edit: end up moving setIsFetchingLocation > setIsFetchingUserLocation into the hook instead of having the component pass it in
There was a problem hiding this comment.
I don't totally agree with the idea that we should never use the word "on" and always use "handle". I think if done correctly, we could use both and each could convey a different meaning. But, that's a higher-level team convo.
# Version 1.3.0 ### Features - Introduce new Facet components including the new Facets (#360), StandardFacet (#370), NumericalFacet (#375), and HierarchicalFacet (#373) components which support more customizability than the previous Facet components. - Support the new RichTextV2 (Lexical) and Markdown fields which have been converted to HTML as FeaturedSnippetDirectAnswers (#379) - Remove the Yext logo from the search bar (#376) - Deprecate LocationBias (#338) component in favor of the new Geolocation component (#336) ### Bug fixes - Improve the UX of the search bar when long autocomplete text is returned which includes the truncation of long text (#378)
This PR adds Geolocation component to the repo, with jest tests and storybook stories.
Note: waiting on product to get SVG design for the default geolocation icon. Edit: Will update in another PR
J=SLAP-2396,SLAP-2397,SLAP-2398
TEST=manual&auto