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: update map pin clusters #3855

Closed
benfurber opened this issue Sep 19, 2024 · 16 comments · Fixed by #4012
Closed

feat: update map pin clusters #3855

benfurber opened this issue Sep 19, 2024 · 16 comments · Fixed by #4012

Comments

@benfurber
Copy link
Member

benfurber commented Sep 19, 2024

Description

Add a shadow effect to the map pin clusters which changes based on the size of the cluster

Mockup and design requirements

Clusters use the primary color of the instance (Project Kamp, Precious Plastic and Fixing fashion). The range of the size for the cluster shadow ranges from 2 (where the shadow is small) to 50 (where the shadow is bigger). Even if the number grows above 50, the shadow should not exceed the maximum spread, otherwise I imagine that on a low zoom level.

image

I have applied the following box-shadow properties (X: 0, Y: 0, blur: 2px, spread varies from 4px to 24px). The color is the main color with 50% opacity. The color of the pins should be configurable and can change if a future decision will be made for the color schemes of the map.

image

Build suggestion

The function to update is createClusterIcon and getClusterSizes in src/pages/Maps/Content/MapView/Sprites.tsx

@6pBit
Copy link

6pBit commented Sep 26, 2024

Hi first time here:) I like to try to solve this issue.

@benfurber
Copy link
Member Author

Welcome @6pBit! Please shout when you have questions.

@benfurber
Copy link
Member Author

Hey @6pBit - how are you getting on?

@6pBit
Copy link

6pBit commented Oct 11, 2024

Been a bit busy the last weeks, but im going to make time to work on it this weekend:)

@y-bhargava
Copy link

Hi @benfurber, I noticed that this PR hasn't been completed yet. If it's still pending and you'd like someone to pick it up, I'd be happy to take over and finish it.

@benfurber
Copy link
Member Author

How are you getting on @6pBit?

Thanks for the offer @y-bhargava! I don't believe we're heard from @6pBit since the last message so how's about let's give them 24 hours from now to respond and if we don't hear back by then the issue is yours. :)

@6pBit
Copy link

6pBit commented Nov 6, 2024

Hi, yes sorry for the late response @benfurber . The dynamic shadow effect feature is implemented, but i am a little bit stuck on where im going to get the color for the type of pin it is. I can set it dynamically, but im not sure where to get the value. I tried reading it from the SVG but that didnt work. Any advise?

@benfurber
Copy link
Member Author

@6pBit There's a theme package which defines a bunch of styling. When using an sx prop, on a color field you can call the color name and it'll Just Work. e.g. <Box sx={{ borderColor: 'darkGrey'}}> will call the hex for darkGrey defined on packages/themes/src/common/commonStyle.ts. I think you'll want the color just called 'primary' defined for each theme on packages/themes/src/<THEME NAME>/styles.ts (e.g. packages/themes/src/project-kamp/styles.ts)

@benfurber
Copy link
Member Author

@y-bhargava Did anything else on the board take your fancy?

johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 19, 2024
@johannes-ross
Copy link
Contributor

I've found a solutions for everything except the border blur of 2px.

There is a solutions, which would just put 2 Pins at the exact same spot, so you can work with "border" instead of outline (which is what I've done). But that solution also requires a bit of a workaround and misuse of one of our libraries which is why I decided against it.

Since this is a pure visual change, writing unit tests wouldn't make sense. I wanted to write a Storybook test, but storybook is sadly outside the root folder, which is why I cannot add a Storybook test.

The Solution I went for is sadly not tested on all themes, since I can't figure out how to change the theme locally. I'm guessing it requires to have a local backend.

My question moving forward is: should I write any kind of tests (for example a unit test, that checks if the HTML is valid) / should I try and Test this with different themes? If no, I can create a pull request. If yes, I might need some assistance.

@pizzaisdavid
Copy link
Collaborator

pizzaisdavid commented Nov 19, 2024

Welcome!

The Solution I went for is sadly not tested on all themes, since I can't figure out how to change the theme locally. I'm guessing it requires to have a local backend.

Maybe things have changed since I last used the project, but I believe there should be a red banner at the top, where you can pick the themes.

I believe it used to work without having a local backend running.

image

@benfurber
Copy link
Member Author

Hey @johannes-ross - Thanks so much for getting involved!

The easiest way to check the different themes atm is to change VITE_THEME in the .env file (the options are documented on the file. Annoyingly there's a vite bug which says the config has been changed...but it hasn't so you still have to stop/start the server.

I agree that for such a small UI change tests aren't important.

Re. the Storybook tests, do you mean the tests in packages/components/? If so I tend to have storybook running in a second terminal to review the storybook while I'm making changes.

@johannes-ross
Copy link
Contributor

Maybe things have changed since I last used the project, but I believe there should be a red banner at the top, where you can pick the themes.

Not in the build that is currently in the master.
image

The easiest way to check the different themes atm is to change VITE_THEME in the .env file

That worked for me.

image
image
image
image

Works with all the Main-Colors.

the Storybook tests, do you mean the tests in packages/components/?

Yes, I would want to add those tests and use the actual components. Sadly, the actual compontens are outside the storybook source folder, which is why I can't use them. So, I think I'm just gonna write a Unit-Test, that checks for the valid CSS.

@mariojsnunes
Copy link
Contributor

So, I think I'm just gonna write a Unit-Test, that checks for the valid CSS.

what if the css changes, do we need to update the tests?
asking because adding tests for this might not be worth the trouble

@johannes-ross
Copy link
Contributor

what if the css changes, do we need to update the tests?

When we change the CSS then yes. I would prefer storybook test but

Yes, I would want to add those tests and use the actual components. Sadly, the actual compontens are outside the storybook source folder, which is why I can't use them.

as explained here, that's sadly not possible.

I've already written the test. I'm gonna commit it to my Fork and if we decide, we don't like it, we just remove it, before we merge :)

johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 20, 2024
johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 21, 2024
johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 21, 2024
johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 21, 2024
johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 21, 2024
@github-project-automation github-project-automation bot moved this from New / Under Discussion to Done in Community Platform Nov 26, 2024
johannes-ross added a commit to johannes-ross/community-platform that referenced this issue Nov 26, 2024
@onearmy-bot
Copy link
Collaborator

🎉 This issue has been resolved in version 2.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

7 participants