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

feat: enable user to share a link to the map that will open a drawer at a specific area id #1210

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

clintonlunn
Copy link
Collaborator

@clintonlunn clintonlunn commented Nov 11, 2024


name: Pull request
about: Create a pull request
title: ''
labels: ''
assignees: ''


What type of PR is this?(check all applicable)

  • refactor
  • feature
  • bug fix
  • documentation
  • optimization
  • other

Description

Previously we did not have a way to share a specific area id in the map view. This PR adds whichever area is currently active in the drawer into the the url. This enables the user to either click share on the url, or click a share button in the drawer which accomplishes the same thing.

Related Issues

Issue #1209

What this PR achieves

Screenshots, recordings

image

Notes

In the future, I'd like to extend this to also the climb id, where the page will open with the relevant area drawer open, and the climb highlighted.

@clintonlunn clintonlunn marked this pull request as ready for review November 18, 2024 16:32
@clintonlunn
Copy link
Collaborator Author

@vnugent i think this PR is about complete. I made sure I am up to date with develop. This required some refactoring of existing camera params, which i've combined into a single useUrlParams in order to deal with the url in a single place. In addition, I've removed the debouncing that was happening for the camera movement, and instead change the url onMoveEnd, which I think gives us more expected behavior.

Copy link
Contributor

@vnugent vnugent left a comment

Choose a reason for hiding this comment

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

I made some inline comments. It's great that you refactor the hooks to its own file. Not high priority (if you can do it in follow up PR) is to clear areaId when the user clicks elsewhere on the map (currently we close the drawer, clearing the current selected area). I think this behavior is consistent with other popular maps.

src/components/ui/ShareButton.tsx Outdated Show resolved Hide resolved
src/app/(maps)/components/FullScreenMap.tsx Outdated Show resolved Hide resolved
src/js/hooks/useUrlParams.tsx Outdated Show resolved Hide resolved

const setActiveFeatureVisual = (feature: ActiveFeature | null, fState: FeatureState): void => {
if (feature == null || mapInstance == null) return
if (feature === null || mapInstance === null) return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change triple === to double catch both null and undefined? (there are several instances). Other than that I'm good to merge.

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.

Store selected area id in url for /maps endpoint
2 participants