-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Migrate Map visualization to React #4278
Conversation
a5d4a5f
to
d54affa
Compare
e2c4383
to
df24bc5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Found bug.
@ranbena noticed that marker cluster icon style is inconsistent depending on "Group by" option. I implemented a fix which IMO looks much better, but now marker group icons look slightly different (see screenshots). It's a kind of breaking change, so I need your opinion - @arikfr, @ranbena, @gabrieldutra - should we keep old version on new one? before:Note: green markers have default style set by Leaflet - settings from "Groups" tab are completely ignored. after:Note: on both screenshots markers use colors defined on "Groups" tab. |
81c3106
to
d38d4cc
Compare
I played around with it and it behaves very sensibly 👍 |
One thing I noticed is that the |
We have something like this in other visualizations (e.g. Chart) - when you choose Auto color - it does not show actual color in editor. |
Good point. I think that in the chart editor it's less confusing, because in the current color picker UI for charts, at least when you click on it, it tells you it's automatic and if it's automatic the preview is empty. Here it shows the "transparent" color icon, which is a bit confusing imo. How about we use an "A" to signal automatic color? And maybe show a tooltip saying the same when hovering over it? |
05f6018
to
bdf281b
Compare
@arikfr How about something like this? |
This is an optional behavior, right? Not sure it will "fit" everywhere where we use the color picker. |
@arikfr Yes, this is optional behavior. I tried to add a tooltip (does not work fine with color picker's popover), tried adding letter "A" (code looked a bit ugly and it still wasn't easy to understand what that mean). I ended up with this variant. |
@kravets-levko ok, let's move on and maybe figure something else if needed. 👍 |
What type of PR is this? (check all applicable)
Description
Related Tickets & Documents
#3301 (Migrate Visualizations to React -> Map)
Mobile & Desktop Screenshots/Recordings (if there are UI changes)