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

Mapbox Component #267

Merged
merged 3 commits into from
Oct 5, 2022
Merged

Mapbox Component #267

merged 3 commits into from
Oct 5, 2022

Conversation

alextaing
Copy link
Contributor

@alextaing alextaing commented Jul 29, 2022

This PR introduces the Mapbox Component. This lays the basic foundation for the component, with the interfaces constructed through discussion with Product.

Here are two relevant slack threads:
https://yext.slack.com/archives/C032CKFARGS/p1658322867803349 (inception)
https://yext.slack.com/archives/C032CKFARGS/p1659449732357219 (more recent)

J=SLAP-2221
TEST=manual

Tested through LocationsPage

Later items will add jest tests and storybook for further testing

NOTE: as of now, local test-site requires an env variable REACT_APP_MAPBOX_API_KEY for map to work properly
For github workflow to work (in future test items), some changes may be needed in the workflow: facebook/create-react-app#9064 (comment)

@alextaing alextaing added the wip label Jul 29, 2022
@alextaing alextaing requested a review from a team as a code owner July 29, 2022 13:39
src/components/LocationBias.tsx Outdated Show resolved Hide resolved
src/components/Mapbox.tsx Outdated Show resolved Hide resolved
src/components/Mapbox.tsx Outdated Show resolved Hide resolved
src/components/Mapbox.tsx Outdated Show resolved Hide resolved
src/components/Mapbox.tsx Outdated Show resolved Hide resolved
if (markerLocation) {
const { latitude, longitude } = markerLocation;
const el = document.createElement('div');
PinComponent && ReactDOM.render(<PinComponent result={result} />, el);
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you using ReactDOM.render 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.

This was one of the things that I wanted to go back and revisit-- I took this small 2 line snippet from davish's example since he implemented it in a way that we can use imported pin components-- Looks like renders the pin to a div, then we insert that div into the Map. Is there a better way to do that? I'll of course look around.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find a good way to do this as Mapbox's marker option takes in HTMLElement only for the DOM element to use as a marker, so this needs to be pre-rendered before passing into new Marker()

src/components/Mapbox.tsx Outdated Show resolved Hide resolved
src/components/Mapbox.tsx Outdated Show resolved Hide resolved
@alextaing
Copy link
Contributor Author

alextaing commented Aug 5, 2022

Hello!! For whoever is working on this after I leave-- thank you! Also, I was thinking about another aspect users might want to configure is the area on the map where pins can show. For example, if for some reason, the user wanted to have a floating window with results, they may want pins to only show on the right side of the window since any pins on the left would be covered by the overlay.

image

Or, if users had different sized pins, they might need to configure the marker margins to make sure no markers get cut off. Also if the map is large, having pins too spread out makes the map hard to read. Overall, I feel like if the marker margins are off, it makes the map look off.

@alextaing
Copy link
Contributor Author

alextaing commented Aug 5, 2022

Another thing that I might want to consider is adding some sort of size styling? Right now, the map fills whatever space it has-- so if the user wants to make a map of a certain size, they have to place it in a div with a specified size. But not necessary, since we're trying to prune prop interfaces.

@alextaing
Copy link
Contributor Author

The pin component was just something that I added in to test compatibility with a custom React component. It is a simplified version of the pin component in Davish's search-locator-starter. It can be removed, and the location page can be refactored when the final PR is ready.

@alextaing
Copy link
Contributor Author

The part with the generic TEntity and the coordinate getter is still a little fuzzy, and I think that there is probably a much better way to implement that idea than what I have at the moment. Also the code organization is weird: I moved the options interfaces to the bottom since they took up a lot of space near the top of the file and looked messy, but they also looks strange no matter where I put them-- maybe they can be extracted into another file?

@yen-tt yen-tt changed the base branch from main to develop September 26, 2022 14:02
@yen-tt yen-tt removed the wip label Sep 26, 2022
test-site/src/pages/LocationsPage.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/hooks/useDebouncedFunction.ts Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
test-site/src/components/MapPin.tsx Show resolved Hide resolved
src/components/MapboxMap.tsx Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
test-site/package.json Show resolved Hide resolved
test-site/src/pages/LocationsPage.tsx Outdated Show resolved Hide resolved
test-site/src/pages/LocationsPage.tsx Show resolved Hide resolved
test-site/src/pages/LocationsPage.tsx Outdated Show resolved Hide resolved
src/components/MapboxMap.tsx Outdated Show resolved Hide resolved
@yen-tt
Copy link
Contributor

yen-tt commented Sep 28, 2022

rewrote history to remove LocationsPage and deleted a comment point to it. Adding it back in another commit.
For now, local test-site requires an env variable REACT_APP_MAPBOX_API_KEY for map to work properly

@yen-tt yen-tt closed this Sep 28, 2022
@yen-tt yen-tt reopened this Sep 28, 2022
@yen-tt yen-tt closed this Sep 28, 2022
@yen-tt yen-tt reopened this Sep 28, 2022
@yext yext deleted a comment from nmanu1 Sep 28, 2022
@yext yext deleted a comment from yen-tt Sep 28, 2022
@yext yext deleted a comment from yen-tt Sep 28, 2022
@nmanu1
Copy link
Contributor

nmanu1 commented Sep 28, 2022

is the failing Snyk license approved by legal?

@yen-tt
Copy link
Contributor

yen-tt commented Sep 28, 2022

is the failing Snyk license approved by legal?

Not sure if alex discussed with Product / run through with legal before using v2 in the pr. Looks like v2 of mapbox is under a different license and pricing model from v1 so I will double check with product

@yen-tt yen-tt changed the base branch from develop to feature/mapbox-component October 4, 2022 14:31
@yen-tt
Copy link
Contributor

yen-tt commented Oct 4, 2022

is the failing Snyk license approved by legal?

Product approved the usage of Mapbox GL JS Version 2.0 and its pricing model, but is still working on the license approval. They gave permission with cutting a beta version -- I will merge this into a feature branch instead for now.

@yen-tt yen-tt merged commit ab88584 into feature/mapbox-component Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants