Conversation
WalkthroughThe changes refactor how map option state is managed and propagated between the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx (1)
150-150: Consider consistent variable naming.The destructured variable is named
colorwhile the setter issetPrimaryColor. For better readability, consider using consistent naming:- const [color, setPrimaryColor] = useRecoilState(primaryColorState); + const [primaryColor, setPrimaryColor] = useRecoilState(primaryColorState);This would also require updating the dependency in the useEffect on line 489.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx(2 hunks)packages/map-template/src/components/MapWrapper/MapWrapper.jsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/map-template/src/components/MapTemplate/MapTemplate.jsx (5)
packages/map-template/src/constants/zoomLevelValues.js (2)
ZoomLevelValues(1-3)ZoomLevelValues(1-3)packages/map-template/src/components/Directions/Directions.jsx (1)
primaryColor(80-80)packages/map-template/src/components/LocationDetails/LocationDetails.jsx (1)
primaryColor(83-83)packages/map-template/src/components/Wayfinding/Wayfinding.jsx (1)
primaryColor(85-85)packages/map-template/src/components/QRCodeDialog/QRCodeDialog.jsx (1)
primaryColor(17-17)
🔇 Additional comments (2)
packages/map-template/src/components/MapWrapper/MapWrapper.jsx (1)
62-62: LGTM! Simplified component interface.The removal of the
onMapOptionsChangecallback parameter aligns with the centralized state management approach implemented inMapTemplate.jsx. This simplification reduces the coupling between components.packages/map-template/src/components/MapTemplate/MapTemplate.jsx (1)
483-489: ```shell
#!/bin/bashSearch for primaryColorState usage across JS and JSX files
rg -A3 -B3 "primaryColorState" --glob ".js" --glob ".jsx"
Search for recoil hook usage in MapTemplate component
rg -A3 -B3 "useRecoil" packages/map-template/src/components/MapTemplate/MapTemplate.jsx
Search for primaryColor prop references in MapTemplate
rg -n "primaryColor" -A3 -B3 packages/map-template/src/components/MapTemplate/MapTemplate.jsx
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@ammapspeople I pushed a small fix. It was discovered that Google Maps does not like when startZoomLevel is not explicitly type of Number. I fixed that. |
ammapspeople
left a comment
There was a problem hiding this comment.
It's a nice cleanup. We should however test the affected props thoroughly and in as many possible combinations as possible.
packages/map-template/src/components/MapTemplate/MapTemplate.jsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Anders Mogensen <am@mapspeople.com>
I will prepare some proper test cases once the merge request is approved 😄 |
What: It was discovered that miTransitionLevel and showRoadNames props are not working.
In meantime I discovered that minZoom level also was not working.
It was due to brandingColor overriding those values.
How:
I made a dedicated useEffect that sets all values at the same time.
Summary by CodeRabbit