Perceived performance improvement with Mapbox Static Images API #331
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.
tmeyer2115
reviewed
Nov 10, 2022
oshi97
reviewed
Nov 10, 2022
src/components/MapboxMap.tsx
Outdated
| const centerAndZoom = `${center[0]},${center[1]},${zoom}`; | ||
| const dimension = `${width}x${height}`; | ||
| return `https://api.mapbox.com/styles/v1/${stylesheet}/static/${centerAndZoom}/${dimension}?access_token=${mapboxAccessToken}`; | ||
| } |
There was a problem hiding this comment.
I hate asking for more legal stuff but do we need to poke product about the static image api pricing? https://docs.mapbox.com/api/maps/static-images/#static-images-api-pricing
the first 50k are free which is nice at least https://www.mapbox.com/pricing/#static-images-api
There was a problem hiding this comment.
per discussion during all hands, tom will run this by product and legal before the feature is merge into develop
|
Current unit coverage is 89.02181562280084% |
oshi97
reviewed
Nov 10, 2022
oshi97
approved these changes
Nov 10, 2022
oshi97
reviewed
Nov 11, 2022
| layout: 'fullscreen', | ||
| percy: { | ||
| enableJavascript: true, | ||
| waitForSelector: '.mapboxgl-map[style=\'visibility: visible;\']' |
There was a problem hiding this comment.
actually, doesn't look like it works with javascript enabled :( . using the storybook play function to force await for the dynamic map doesn't work either
There was a problem hiding this comment.
made an item for it: https://yexttest.atlassian.net/browse/SLAP-2462
oshi97
approved these changes
Nov 11, 2022
nmanu1
reviewed
Nov 11, 2022
oh I think WCAG test wasn't updated in previous PR along with the percy snapshot test, to also accept MAPBOX_API_KEY environment variable since wcag gh action wasn't running at all -- we should consider triggering WCAG for merging to feature branch too... |
nmanu1
reviewed
Nov 16, 2022
yen-tt
added a commit
to yext/slapshot-reusable-workflows
that referenced
this pull request
Nov 16, 2022
When testing wcag in search-react-ui repo, the mapbox API key is required to use Mapbox GL in storybook. Updated the workflow to optionally accept a mapbox API key as an env variable for the build step. J=none TEST=manual tested in this PR: yext/search-ui-react#331, where wcag error `An API access token is required to use Mapbox GL.` is no longer present after using the workflow from this branch.
nmanu1
reviewed
Nov 16, 2022
nmanu1
reviewed
Nov 16, 2022
that is fixed in this PR: #337 for wcag related issues coming from the existing mapbox stories. will merge the change to this branch after. |
paired with the changes in this [PR](yext/slapshot-reusable-workflows#22) in WCAG workflow, this PR updates the WCAG github action in the repo to pass in the mapbox key. Also updated the wcag test to exclude checking elements coming from mapbox canvas container as any potential violations coming from there is outside of our repo's control. WCAG github action also run on pull request to feature branch now. J=SLAP-2458 TEST=auto see that WCAG github action now passes
In addition to the changes in the [workflow PR](yext/slapshot-reusable-workflows#24), this up updates run-tests github action and coverage github action to pass mapbox api key so visual coverage test works as expected. J=SLAP-2467 TEST=auto see that run-tests github action and coverage github action passed -- looked into the logs, no false positive / errors related to visual coverage.
nmanu1
approved these changes
Nov 18, 2022
|
putting wip tag to exclude this from our PR dashboard (even though the PR is done) |
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.
This PR update mapbox component to use the Mapbox Static Images API to improve the perceived performance for users -- i.e immediately show a static image while Mapbox GL JS works in the background. From lighthouse analysis, this is generally similar to the performance before this change, occasionally a couple points increase. (overall ~86)
Note: the API only support width and height up to 1280 pixels, and will response with 422 error code otherwise. We could attempt to stretch the image to cover the space (e.g. such as using background-cover css style) but it wouldn't match with the display of the actual map on load (without initial search) so this PR currently will not use static image API unless the dimension of the map is within the restriction.
J=SLAP-2434
TEST=manual&auto
load location page and see that a static image is fetched over network and display until the canvas for the interactive map is loaded, then that become visible to the user instead.
added a story for map static image