Mapbox component #332
Merged
Mapbox component #332
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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)
added jest tests for Mapbox component, specifically for the two prop fields: `getCoordinate` and `onDrag`. The remaining props are best tested through storybook. note that `mapbox-gl` is mocked as it seems to use some web browser functionality on initialization that is not supported in jest test environment (jsdom). Without mocking, jest would present an error `Error: Failed to initialize WebGL.`. - tried third party libraries `jest-webgl-canvas-mock` and `jest-canvas-mock` still result in more window properties access related errors (`[TypeError: e.window.Worker is not a constructor]`, which could be resolve by [manually mocking Worker](jestjs/jest#3449), `[TypeError: this.target.addEventListener is not a function]`...). - Looked into how pageJS test mapbox provider in their map component: the map wrapper component invoke a [load function](https://github.com/yext/pages/blob/main/packages/pages/src/components/map/map.tsx#L112) to construct [a script tag](https://github.com/yext/components-tsx-maps/blob/main/src/Providers/Mapbox.js#L137) from `yext/components-tsx-maps` and the assertions in the unit tests seem to check for the rendering of the wrapper component only and not waiting for the script to load. No content/interaction of the map was tested from my understanding. - Looked into how mapbox-gl-js does their own testing: they had a couple util files to set up the environment for their tests (mock requests, [mock HtmlCanvas/WebLG in window](https://github.com/mapbox/mapbox-gl-js/blob/main/src/util/window.js) object, etc.). I decided this complexity is probably unnecessary to add to the repo but we can discuss if other may think differently. We can have jest strictly test new isolated functionalities added from the component and leave all the UI rendering and mapbox interaction to storybook tests. SLAP-2222 TEST=auto new jest tests passed
Add react-dom as a peer dependency as it's now use in Mapbox component. Note that, ReactDOM.render is deprecated in React 18, which now have a new client render function `createRoot`. For now, a warning will display in console informing user that the app will behave as if it’s running React 17 until the code switch to the new API. A new item will be created to address this issue. TEST=manual `npm pack` the component lib. Created a new React app using react 17/18, see that react-dom is installed as peer dep (automatically install with npm >=7) and mapbox component is display as expected (there's a warning in React 18).
single marker story multiple marker story custom marker story - follow docs.mapbox.com guide to add a simple popup pin J=SLAP-2223 TEST=manual serve and view stories
This PR uses a dynamic imports with a .catch fallback for when react-dom/client does not exist.
MapboxMap code had to be tweaked due to differences between ReactDOM.render and ReactDOM.createRoot().render(). Namely, createRoot() does not modify the container element unless it already exists on the physical page. This means that we can't call document.creatElement('div'), use createRoot, and THEN attach it to the mapbox map. Instead, we have to attach the div to the map first.
This also seems it make it more difficult to unit test, my guess would be because createRoot does not do anything on the server. For now I use some jest.mocks to ensure the right methods are being called in the right environments.
TEST=manual,auto
storybook can start up (react 17)
test site can display custom pin in both react 17 and 18
…)" This reverts commit 0d78e7e.
|
WCAG tests will be address through another item (this also affect the visual coverage test). Mapbox license flagged by snyk will be marked ignored (approved by legal) once merge to develop. |
|
maybe I missed it, but why is the commit for using createRoot (#319) reverted? |
Hey Nidhi this is the writeup I made last week for why we can't support 17 and 18 simultaneously |
nmanu1
approved these changes
Nov 15, 2022
This was referenced Nov 15, 2022
Merged
tmeyer2115
added a commit
that referenced
this pull request
Dec 15, 2022
### Features - Default behavior of `FilterSearch` was changed to better support Locators and Doctor Finders. Additionally, a new `onSelect` prop was added to the Component. The `searchOnSelect` prop is now deprecated. (#323, #343, #333) - A new CSS bundle without the Tailwind resets is exported. (#322) - We've added a `MapboxMap` Component, powered by v2 of their JavaScript API. (#332) ### Changes - Assorted updates to improve our GH Actions. - Styling of Facet Headers is now exposed in `FilterGroupCssClasses`. (#321) ### Bug Fixes - Vulnerabilities were addressed for the repo and its test-site. - Fixed the Dropdown Component to invoke `preventDefault` only when it is active. (#307) - Corrected a small error in the generation of SSR Hydration IDs. (#315)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
merge mapbox component feature to develop branch