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: Custom zoom buttons #6974

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from
Draft

feat: Custom zoom buttons #6974

wants to merge 14 commits into from

Conversation

VIKTORVAV99
Copy link
Member

@VIKTORVAV99 VIKTORVAV99 commented Jul 11, 2024

Issue

There was two todos and AVO-342.

Description

Uses a <MapProvider> and later the useMap() hook to implement our own zoom buttons, this allows us to keep a unified design, remove the maplibre css and implement the aria labels that was mentioned in the todo comments.

Note

These icons are a mess, being in different sizes and from different libraries with different styles. I and Alex agreed that there should be an operate PR to fix this later on.

Closes: AVO-342

Important

We can probably use the useMap() hook in more places to simplify our logic a bit but that's something for a later PR.

Preview

New:

image

Old:

image

Double check

  • I have run pnpx prettier@2 --write . and poetry run format in the top level directory to format my changes.

@madsnedergaard
Copy link
Member

These icons are a mess, being in different sizes and from different libraries with different styles. I and Alex agreed that there should be an operate PR to fix this later on.

Yes, we should align these when possible, but be aware that we cannot get them from the same package so we do need to maintain our own custom icons here. They could be aligned and stylised in one common way though :)

@VIKTORVAV99
Copy link
Member Author

These icons are a mess, being in different sizes and from different libraries with different styles. I and Alex agreed that there should be an operate PR to fix this later on.

Yes, we should align these when possible, but be aware that we cannot get them from the same package so we do need to maintain our own custom icons here. They could be aligned and stylised in one common way though :)

Why can't we get them from the same package?
I'm just talking about the button icons here and all of them appears to be available in FontAwesome6 (free).

@madsnedergaard
Copy link
Member

These icons are a mess, being in different sizes and from different libraries with different styles. I and Alex agreed that there should be an operate PR to fix this later on.

Yes, we should align these when possible, but be aware that we cannot get them from the same package so we do need to maintain our own custom icons here. They could be aligned and stylised in one common way though :)

Why can't we get them from the same package? I'm just talking about the button icons here and all of them appears to be available in FontAwesome6 (free).

Because they don't have the same icons necessarily - see the translation and solar icons that look quite different in FontAwesome. Not saying we shouldn't change the icons, but I wanted to clarify why we in the past decided not to use the same icon package :)

@VIKTORVAV99
Copy link
Member Author

These icons are a mess, being in different sizes and from different libraries with different styles. I and Alex agreed that there should be an operate PR to fix this later on.

Yes, we should align these when possible, but be aware that we cannot get them from the same package so we do need to maintain our own custom icons here. They could be aligned and stylised in one common way though :)

Why can't we get them from the same package? I'm just talking about the button icons here and all of them appears to be available in FontAwesome6 (free).

Because they don't have the same icons necessarily - see the translation and solar icons that look quite different in FontAwesome. Not saying we shouldn't change the icons, but I wanted to clarify why we in the past decided not to use the same icon package :)

That's why it's a separate PR, we need to figure out exactly which icons to use. I use the Sun and Moon icon from FontAwesome6 in #6982 which are the ones that Alex have used in the Figma designs (but for the charts). We might want to align it here as well but we should probably talk it through first.

Copy link
Member

@madsnedergaard madsnedergaard left a comment

Choose a reason for hiding this comment

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

Some comments, but so nice to get rid of the hacky implementation :)

web/src/components/Button.tsx Outdated Show resolved Hide resolved
web/src/features/map-controls/MapButton.tsx Show resolved Hide resolved
web/src/features/map-controls/MapButton.tsx Outdated Show resolved Hide resolved
@@ -83,58 +83,6 @@
transform: translateX(var(--radix-toast-swipe-end-x));
Copy link
Member

Choose a reason for hiding this comment

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

So nice to get rid of all this crap :D

@VIKTORVAV99
Copy link
Member Author

It seems like the wind layer stopped working...?
Which I don't get cause the solar layer is still working as expected so it's not the buttons that broke it as the logic is shared between those.

@madsnedergaard
Copy link
Member

It seems like the wind layer stopped working...? Which I don't get cause the solar layer is still working as expected so it's not the buttons that broke it as the logic is shared between those.

Strange indeed, it works on other preview branches so it's something specifically here that is causing it 🤔

@VIKTORVAV99
Copy link
Member Author

It seems like the wind layer stopped working...? Which I don't get cause the solar layer is still working as expected so it's not the buttons that broke it as the logic is shared between those.

Strange indeed, it works on other preview branches so it's something specifically here that is causing it 🤔

So the wind layer stops working even after the first commit in this PR that only changes the zoom buttons and adds the map provider. So it's probably something to do with the map provider itself. Which makes me think there is an underlying issue somewhere in the wind layer (that don't exist in the solar layer).

I'll see if I can fix the wind layer in a separate PR. However I'm curious why we are relying on a bunch of use effect hooks to control the on and off state instead of conditionally rendering the whole component?

Got any insight there?

@madsnedergaard
Copy link
Member

Can't remember anything about it at all, but here be dragons and we have struggled with it - happy to help on it :)

@VIKTORVAV99
Copy link
Member Author

Can't remember anything about it at all, but here be dragons and we have struggled with it - happy to help on it :)

I'll see if I can make them always on and then conditionally render them instead. Hopefully that solves the issue and makes it easier for us to unify the logic and get rid of a few use effect hooks.

@madsnedergaard
Copy link
Member

Hey, should we put this back in draft? Seems like there's still quite some work to be done to ensure the wind layers work 😬

@madsnedergaard madsnedergaard removed their request for review September 18, 2024 12:51
@VIKTORVAV99
Copy link
Member Author

Hey, should we put this back in draft? Seems like there's still quite some work to be done to ensure the wind layers work 😬

Yeah we might as well. I still have no idea why they actually break 😅

They shouldn't IMO but I haven't had time to look into it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants