react-map-gl icon indicating copy to clipboard operation
react-map-gl copied to clipboard

Improve docs and hooks-example for `useMap` and `MapProvider`

Open tordans opened this issue 2 years ago • 8 comments

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.

tordans avatar Feb 09 '23 12:02 tordans

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 https://github.com/mapbox/mapbox-gl-js/issues/11816

benderlidze avatar Feb 09 '23 13:02 benderlidze

@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:
   // {

tordans avatar Feb 10 '23 06:02 tordans

I converted this do a draft since it will need to be re applied to the latest version of the docs and code.

tordans avatar Sep 04 '23 19:09 tordans

@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!

Bonkles avatar Apr 25 '24 19:04 Bonkles

@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.

tordans avatar Apr 26 '24 07:04 tordans

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

tordans avatar Jul 06 '24 12:07 tordans

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

Pessimistress avatar Jul 07 '24 17:07 Pessimistress

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

tordans avatar Jul 10 '24 15:07 tordans