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

Improve docs and hooks-example for useMap and MapProvider #2118

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tordans
Copy link
Contributor

@tordans tordans commented Feb 9, 2023

I took some time to learn useMap and MapProvider today. The changes in this PR represent what I would have needed to understand the behavior of useMap and MapProvider faster. I ran into issues because I picked a name for my map id that collided with in import. And I used useMap right in the component which rendered the MapProvider, which – seems obvious now – does not work.

The commits are kept tiny to give them all a "why I added this" commit message.

@benderlidze
Copy link

And const {current: map} = useMap(); map is always undefined. It only works if you use it with the map id only.
Like const { current: map, mymap } = useMap(); mymap is ok, map - undefined. Here is a link to the discussion at Mapbox mapbox/mapbox-gl-js#11816

@tordans
Copy link
Contributor Author

tordans commented Feb 10, 2023

@benderlidze it does work when used in Control2 which is a child of Map. I expected it to, since that is the only place where current is defined at all.

Console:

image

Diff to this PR for local Testing:

diff --git a/examples/get-started/hook/controls.js b/examples/get-started/hook/controls.js
index e473accd..42503d6d 100644
--- a/examples/get-started/hook/controls.js
+++ b/examples/get-started/hook/controls.js
@@ -4,7 +4,7 @@ import {useCallback, useState, useEffect} from 'react';
 import {useMap} from 'react-map-gl';

 export default function Controls() {
-  const demo = useMap();
+  const {current: demo} = useMap();
   console.log('Controls', {demo});
   // First render:
   // {
diff --git a/examples/get-started/hook/controls2.js b/examples/
get-started/hook/controls2.js
index 619c5847..79e80f8d 100644
--- a/examples/get-started/hook/controls2.js
+++ b/examples/get-started/hook/controls2.js
@@ -9,7 +9,7 @@ import {useMap} from 'react-map-gl';
 // that references the containing map.
 // See /docs/api-reference/use-map.md
 export default function Controls2() {
-  const demo = useMap();
+  const {current: demo} = useMap();
   console.log('Controls2', {demo});
   // First render:
   // {
diff --git a/examples/get-started/hook/map.js b/examples/get-st
arted/hook/map.js
index 0712d16e..95496d7f 100644
--- a/examples/get-started/hook/map.js
+++ b/examples/get-started/hook/map.js
@@ -7,7 +7,7 @@ import Controls2 from './controls2';
 const MAPBOX_TOKEN = ''; // Set your mapbox token here

 export default function MapView() {
-  const demo = useMap();
+  const {current: demo} = useMap();
   console.log('MapView', {demo});
   // First render:
   // {

Copy link
Collaborator

@Pessimistress Pessimistress left a comment

Choose a reason for hiding this comment

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

I have no objection to the doc updates, but I don't think the console.log statements belong in the examples. If you think they are helpful, one way to do it is to add the code snippet to the documentation page under useMap, then link to the documentation from the example apps.

@tordans tordans marked this pull request as draft September 4, 2023 19:26
@tordans

This comment was marked as outdated.

@Bonkles
Copy link

Bonkles commented Apr 25, 2024

@tordans this PR saved my bacon in a side project I just started. These doc additions about the map id being crucial escaped me too!

@tordans tordans force-pushed the usemap-docs-example branch 2 times, most recently from 3a1e058 to df56fc1 Compare April 26, 2024 06:59
@tordans tordans requested a review from Pessimistress April 26, 2024 06:59
@tordans tordans marked this pull request as ready for review April 26, 2024 06:59
@tordans
Copy link
Contributor Author

tordans commented Apr 26, 2024

@Bonkles glad to hear this helped. I took your comment as a nudge to finally update this PR.

@Pessimistress

I have no objection to the doc updates, but I don't think the console.log statements belong in the examples. If you think they are helpful, one way to do it is to add the code snippet to the documentation page under useMap, then link to the documentation from the example apps.

I added the console log as a code comment block in the example. Does that work for you? I find it helpful to have this in context of the component (tree).

The PR is rebased on current master and wordings are updated slightly since your last review.

examples/get-started/hook/controls.jsx Outdated Show resolved Hide resolved
examples/get-started/hook/controls2.js Outdated Show resolved Hide resolved
examples/get-started/hook/map.jsx Outdated Show resolved Hide resolved
@tordans tordans force-pushed the usemap-docs-example branch from 396bd88 to 9fc4fcb Compare July 6, 2024 11:59
@tordans tordans requested a review from Pessimistress July 6, 2024 12:00
@tordans
Copy link
Contributor Author

tordans commented Jul 6, 2024

@Pessimistress thanks for the review, those changes make it easier to understand! I rebased to resolve the conflicts. Anything else I can do?

Copy link
Collaborator

@Pessimistress Pessimistress left a comment

Choose a reason for hiding this comment

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

Thank you!

@Pessimistress
Copy link
Collaborator

@tordans Could you fix the lint errors introduced by your change?

tordans and others added 5 commits July 10, 2024 17:07
- Clarify that the Map id is required (only) when using MapProvider / useMap. Link those docs.
- Note on picking a unique name for the id
This gets rid of Typescript complaining about possible undefined `mymap`. This also updates the other guard to be inline to keep the example clean.
- Show how useMap works on first render, second render
- Hint at useMap not working outside

Add console statements and code comments to reference how useMap works in the different situations. The code comment help understand that useMap returns undefined first; and what the shape of the returned object looks like.

The controls2 component shows how the "current" works when useMap is used inside a Map component.

All console logs are formatted as code comments so they are easy to reproduce but don't actually log.
```
/home/runner/work/react-map-gl/react-map-gl/examples/get-started/hook/controls.jsx
Error:   53:35  error  Expected to return a value at the end of arrow function  consistent-return

/home/runner/work/react-map-gl/react-map-gl/examples/get-started/hook/controls2.js
Error:   1:9  error  'useMap' is defined but never used  no-unused-vars

/home/runner/work/react-map-gl/react-map-gl/examples/get-started/hook/map.jsx
Error:   2:14  error  'useMap' is defined but never used  no-unused-vars
```
@tordans tordans force-pushed the usemap-docs-example branch from 47b370c to e92bd3d Compare July 10, 2024 15:07
@tordans
Copy link
Contributor Author

tordans commented Jul 10, 2024

@Pessimistress I fixed the linter issues and rebased on current master. But the action requires your approval before running.

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.

4 participants