OpenBeta / open-tacos

Rock climbing route catalog (openbeta.io)
https://openbeta.io
GNU Affero General Public License v3.0
110 stars 111 forks source link

Map layers selector #1108 #1118

Closed d-k-lippert closed 2 months ago

d-k-lippert commented 2 months ago

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


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

Description

Related Issues

Not that i know

Issue #1108

What this PR achieves

the pr implements a map layer style switcher by using RadiX UI Hover Card When clicked on a certain map tile the style of the whole map switches. So far 4 different styles are implemented (see MapSelector.tsx):

  1. outdoor
  2. dataviz
  3. basic (minimalist)
  4. satellite

I knew only the minimalist, outdoor, satellite were required, but in the mapselector there were already 2 different styles (outdoor and dataviz) so i added the ones that were missing instead of deleting. Also the dataviz style was used in the implemented in the RecentContributionMap.tsx.

What does this PR:

Screenshots, recordings

image

Notes

As for the implementation of the map layer icons (tiles) image

i needed to get the imageUrls to show example images of the map styles. As of now I got them from here: https://docs.maptiler.com/sdk-js/api/map-styles/#referencemapstyle (copied the image url of the respective map layer style)

vercel[bot] commented 2 months ago

@d-k-lippert is attempting to deploy a commit to the openbeta-dev Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
open-tacos ✅ Ready (Inspect) Visit Preview Apr 18, 2024 2:33pm
vnugent commented 2 months ago

Looking great! What do you think if we give each map style a user-friendly name? Mainly, dataviz -> "Minimal", basic -> Standard (or Classic). Or you think the current names are as fine as they are? I'd be happy to merge the PR as it is.

musoke commented 2 months ago

This is really nice! The maps have come a long way in the last few months.

One thing: I failed to change the map style when looking at the preview on android. Both Chrome 123.0.6312.99 and Firefox 125.0 (Build #2016013951), 32e0da40b1+.

Unclear if it's a bug or user error (likely the latter but ideally this would be obvious some someone moderately techsavy). I was tapping the small box that shiws the current map style.

Also, I agree with @vnugent about giving the styles user-friendly names. If you do that I'd suggest aligning the names with the defaults (i.e. consider only using the name "standard" if it's the default).

d-k-lippert commented 2 months ago

This is really nice! The maps have come a long way in the last few months.

One thing: I failed to change the map style when looking at the preview on android. Both Chrome 123.0.6312.99 and Firefox 125.0 (Build #2016013951), 32e0da40b1+.

Unclear if it's a bug or user error (likely the latter but ideally this would be obvious some someone moderately techsavy). I was tapping the small box that shiws the current map style.

Also, I agree with @vnugent about giving the styles user-friendly names. If you do that I'd suggest aligning the names with the defaults (i.e. consider only using the name "standard" if it's the default).

Now if I think about it, that might be related to the Radix UI Hover Card component. The box opens only when hovered over. Which is a problem when opened on a smartphone. I wasn't really thinking about it when implementing 🤦‍♂️. But it should be easy to trigger the box also when the tile is clicked.

musoke commented 2 months ago

Long term comment, mostly notes for myself in the distant future.

It looks straightforward to allow self-hosted map styles too. This will be helpful if we render tiles specifically for openbeta, similar to what https://github.com/SomeoneElseOSM/SomeoneElse-style does for walking routes in rural UK.

d-k-lippert commented 2 months ago

This is really nice! The maps have come a long way in the last few months. One thing: I failed to change the map style when looking at the preview on android. Both Chrome 123.0.6312.99 and Firefox 125.0 (Build #2016013951), 32e0da40b1+. Unclear if it's a bug or user error (likely the latter but ideally this would be obvious some someone moderately techsavy). I was tapping the small box that shiws the current map style. Also, I agree with @vnugent about giving the styles user-friendly names. If you do that I'd suggest aligning the names with the defaults (i.e. consider only using the name "standard" if it's the default).

Now if I think about it, that might be related to the Radix UI Hover Card component. The box opens only when hovered over. Which is a problem when opened on a smartphone. I wasn't really thinking about it when implementing 🤦‍♂️. But it should be easy to trigger the box also when the tile is clicked.

Okay, the topic above isn't that straightforward (at least to me). I played around with it and wasn't able to implement it using the Hover Card. Maybe it's better if we use the PopOver instead? @vnugent @musoke Please let me know what u think

musoke commented 2 months ago

I think you're right about HoverCard not working, this is intentional according to the issue where it was proposed. The radix devs suggested PopOver in a similar issue about ToolTip, so I think your idea will work.

vnugent commented 2 months ago

I completely missed hovering AND mobile when writing up the original ticket. Yes, PopOver is a better way to go. Looks like @d-k-lippert already pushed the change. Can you please check again on your mobile @musoke?

musoke commented 2 months ago

Screenshot_20240417_170621

Works for me! The PopOver is a little wide though. Can it be limited to the width of the screen?

d-k-lippert commented 2 months ago

Screenshot_20240417_170621

Works for me! The PopOver is a little wide though. Can it be limited to the width of the screen?

Sure, i will look into it :)

d-k-lippert commented 2 months ago

I changed it to a vertical layout - actually prefer this one to the previous one, because it has more consistent sizes: for screens wider than 768px image for screens smaller than that: image Pls let me know what you think about it.

musoke commented 2 months ago

I can't find the vercel preview but the screenshots look good to me 🙂

vnugent commented 2 months ago

@all-contributors add @d-k-lippert for code & ideas

allcontributors[bot] commented 2 months ago

@vnugent

I've put up a pull request to add @d-k-lippert! :tada: