Add center prop and logic around it #436

matbmapspeople · 2025-01-09T10:14:14Z

I added possibility to read center's value from the URL.

Based on this value and other URL prop values I created a logic where the map should be centred.

packages/map-template/src/components/MapTemplate/MapTemplate.jsx

packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx

andreeaceachir142 · 2025-01-09T12:54:17Z

@ammapspeople I've added a few comments here which stood out at a first glance, to save you some time 😄

ammapspeople

I'll pause reviewing this until the build error mentioned in the Jira ticket is resolved.

ammapspeople · 2025-01-09T12:37:22Z

packages/map-template/src/components/MapTemplate/MapTemplate.jsx

@@ -553,6 +556,13 @@ function MapTemplate({ apiKey, gmApiKey, mapboxAccessToken, venue, locationId, p
        setSearchExternalLocations(searchExternalLocations);
    }, [searchExternalLocations]);

+    /*
+     * TODO


Please do 😁

ammapspeople · 2025-01-09T12:37:47Z

packages/map-template/src/components/MapTemplate/MapTemplate.jsx

@@ -92,8 +93,9 @@ defineCustomElements();
 * @param {boolean} [props.showRoadNames] - A boolean parameter that dictates whether Mapbox road names should be shown. By default, Mapbox road names are hidden when MapsIndoors data is shown. It is dictated by `mi-transition-level` which default value is 17.
 * @param {boolean} [props.showExternalIDs] - Determine whether the location details on the map should have an external ID visible. The default value is set to false.
 * @param {boolean} [props.searchExternalLocations] - If you want to perform search for external locations in the Wayfinding mode. If set to true, Mapbox/Google places will be displayed depending on the Map Provider you are using. If set to false, the results returned will only be MapsIndoors results. The default is true.
+ * @param {string} [props.center] - TODO


Needs explanation

ammapspeople · 2025-01-09T12:38:04Z

packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx

@@ -36,6 +36,7 @@ import getBooleanValue from "../../helpers/GetBooleanValue.js";
 * @param {boolean} [props.showExternalIDs] - Determine whether the location details on the map should have an external ID visible. The default value is set to false.
 * @param {boolean} [props.showRoadNames] - A boolean parameter that dictates whether Mapbox road names should be shown. By default, Mapbox road names are hidden when MapsIndoors data is shown. It is dictated by `mi-transition-level` which default value is 17.
 * @param {boolean} [props.searchExternalLocations] - If you want to perform search for external results in the Wayfinding mode. If set to true, Mapbox/Google places will be displayed depending on the Map Provider you are using. If set to false, the results returned will only be MapsIndoors results. The default is true.
+ * @param {string} [props.center] - TODO


Needs explanation.

ammapspeople · 2025-01-09T12:39:28Z

packages/map-template/src/hooks/useMapBoundsDeterminer.js

    const venuesInSolution = useRecoilValue(venuesInSolutionState);
    const currentPitch = useRecoilValue(currentPitchSelector);
    const venueWasSelected = useRecoilValue(venueWasSelectedState);
    const [kioskLocationDisplayRuleWasChanged, setKioskLocationDisplayRuleWasChanged] = useState(false);
+    let centerValue = useRecoilValue(centerState);


Can we name it center to follow the existing pattern?

ammapspeople · 2025-01-09T12:41:32Z

packages/map-template/src/hooks/useMapBoundsDeterminer.js

+            getDesktopPaddingBottom().then(desktopPaddingBottom => {
+                desktopPadBottom = desktopPaddingBottom;
+            });
+
+            getMobilePaddingBottom().then(mobilePaddingBottom => {
+                mobilePadBottom = mobilePaddingBottom
+            });
+
+            getDesktopPaddingLeft().then(desktopPaddingLeft => {
+                desktopPadLeft = desktopPaddingLeft;
+            });


Use Promise.all() instead of running them one by one. Then you'll get an array with all the values when all have resolved. See for example the onLocationClicked function in the Search component.

matbmapspeople · 2025-01-09T13:53:49Z

I now addressed all your comments @andreeaceachir142 and @ammapspeople

packages/map-template/src/components/MapTemplate/MapTemplate.jsx

ammapspeople · 2025-01-10T06:53:22Z

packages/map-template/src/hooks/useMapBoundsDeterminer.js

+            const zoomLevel = startZoomLevel ?? 16;
+
+
+            Promise.all([getDesktopPaddingBottom(), getDesktopPaddingLeft(), getMobilePaddingBottom()]).then(([desktopPaddingBottom, desktopPaddingLeft, mobilePaddingBottom]) => {


Won't we risk that the "pad" variables are null since this is asynchronous and the code following it is immediately executed?

I reversed the code as it was before. Because version before works.

Co-authored-by: Anders Mogensen <[email protected]>

ammapspeople · 2025-01-10T08:11:19Z

The bounds are not fit to venue when no center prop provided:

ammapspeople · 2025-01-10T08:39:04Z

The center prop is not reacting to changes runtime. It should.

Kapture.2025-01-10.at.09.36.53.mp4

matbmapspeople · 2025-01-10T11:19:37Z

I believe I addressed all your comments @ammapspeople
I would appreciate @andreeaceachir142 if you can resolve yours if I managed to fix your issues :)

ammapspeople · 2025-01-14T08:13:06Z

packages/map-template/src/components/MapsIndoorsMap/MapsIndoorsMap.jsx

@@ -36,6 +36,7 @@ import getBooleanValue from "../../helpers/GetBooleanValue.js";
 * @param {boolean} [props.showExternalIDs] - Determine whether the location details on the map should have an external ID visible. The default value is set to false.
 * @param {boolean} [props.showRoadNames] - A boolean parameter that dictates whether Mapbox road names should be shown. By default, Mapbox road names are hidden when MapsIndoors data is shown. It is dictated by `mi-transition-level` which default value is 17.
 * @param {boolean} [props.searchExternalLocations] - If you want to perform search for external results in the Wayfinding mode. If set to true, Mapbox/Google places will be displayed depending on the Map Provider you are using. If set to false, the results returned will only be MapsIndoors results. The default is true.
+ * @param {string} [props.center] - Specifies the coordinates where the map should load, represented as latitude and longitude values separated by a comma. If the specified coordinates intersect with a Venue, that Venue will be selected in the Venue Selector.


Maybe use the same phrasing pattern as for the venue and locationId prop: "If you want the map to center on a specific coordinate, provide [...]"

ammapspeople · 2025-01-14T09:57:50Z

packages/map-template/src/hooks/useMapBoundsDeterminer.js

+            const centerPoint = { geometry: { type: 'Point', coordinates: [latitude, longitude] } };
+
+            // Returns Venue that intersects with center prop.
+            const intersectingVenueWithCenterPoint = venuesInSolution.find(venue => {


This is not necessary to calculate at this point in time, since the value is only used in some cases. Postpone to be calculated only when we know it's going to be used.

ammapspeople · 2025-01-14T10:02:28Z

packages/map-template/src/hooks/useMapBoundsDeterminer.js

+                if (venueWasSelected) {
+                    // If switching Venues, while having center prop defined, set center prop to undefined.
+                    // It is required to be able to switch between Venues inside Venue Selector.
+                    center = undefined;


We should not set Recoil atoms to undefined. I guess that would stop the reactivity of it. Instead use a useRecoilState instead of a useRecoilValue hook for the center atom to get access to a setter function.

matbmapspeople · 2025-01-14T11:20:04Z

I applied your suggestions @ammapspeople

matbmapspeople added 6 commits January 8, 2025 08:08

update: npm i

379f113

chore: add center to webcomponent.js

2e2a7ec

add: define center atom

fa47955

chore: create a setter inside MapTemplate.jsx

087d8d3

add: center to MapsIndoorsMap

2f21963

chore: improve determineMapBounds function so it respects center prop

2d254d2

matbmapspeople added the patch label Jan 9, 2025

matbmapspeople requested a review from ammapspeople January 9, 2025 10:14

matbmapspeople self-assigned this Jan 9, 2025

matbmapspeople added minor and removed patch labels Jan 9, 2025

andreeaceachir142 reviewed Jan 9, 2025

View reviewed changes

ammapspeople reviewed Jan 9, 2025

View reviewed changes

matbmapspeople added 3 commits January 9, 2025 14:45

fix: lerna error

95e57bb

fix: jsdocs

4324c6f

fix: use promise and fix naming

3dc341b

ammapspeople reviewed Jan 10, 2025

View reviewed changes

packages/map-template/src/components/MapTemplate/MapTemplate.jsx Outdated Show resolved Hide resolved

ammapspeople reviewed Jan 10, 2025

View reviewed changes

Update packages/map-template/src/components/MapTemplate/MapTemplate.jsx

ef705ff

Co-authored-by: Anders Mogensen <[email protected]>

matbmapspeople added 2 commits January 10, 2025 12:10

fix: runtime changes and zoom level

cd15493

fix: null padding values

76ca32c

ammapspeople reviewed Jan 14, 2025

View reviewed changes

matbmapspeople added 2 commits January 14, 2025 12:16

chore: improve JSDocs

c75eb49

chore: use recoilstate and move intersecting point later in the code

1858817

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add center prop and logic around it #436

Add center prop and logic around it #436

matbmapspeople commented Jan 9, 2025

andreeaceachir142 commented Jan 9, 2025

ammapspeople left a comment

ammapspeople Jan 9, 2025

ammapspeople Jan 9, 2025

ammapspeople Jan 9, 2025

ammapspeople Jan 9, 2025

ammapspeople Jan 9, 2025

matbmapspeople commented Jan 9, 2025

ammapspeople Jan 10, 2025

matbmapspeople Jan 10, 2025

ammapspeople commented Jan 10, 2025

ammapspeople commented Jan 10, 2025

matbmapspeople commented Jan 10, 2025

ammapspeople Jan 14, 2025

ammapspeople Jan 14, 2025

ammapspeople Jan 14, 2025

matbmapspeople commented Jan 14, 2025

		const zoomLevel = startZoomLevel ?? 16;


		Promise.all([getDesktopPaddingBottom(), getDesktopPaddingLeft(), getMobilePaddingBottom()]).then(([desktopPaddingBottom, desktopPaddingLeft, mobilePaddingBottom]) => {

Add center prop and logic around it #436

Are you sure you want to change the base?

Add center prop and logic around it #436

Conversation

matbmapspeople commented Jan 9, 2025

andreeaceachir142 commented Jan 9, 2025

ammapspeople left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matbmapspeople commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ammapspeople commented Jan 10, 2025

ammapspeople commented Jan 10, 2025

matbmapspeople commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matbmapspeople commented Jan 14, 2025