-
Notifications
You must be signed in to change notification settings - Fork 9
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 #332
Mapbox component #332
Conversation
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 |
### 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)
merge mapbox component feature to develop branch