Closed charliemcgrady closed 1 month ago
Was it intentional to prevent the user to from "de-highlighting" a theme? I am a huge fan of the feature, but I also think that it removes some views from the current map.
Going to merge the address work and new tiles work PRs first @charliemcgrady and then I'll ask you to address the conflicts- then I can give you a more formal review
Going to merge the address work and new tiles work PRs first @charliemcgrady and then I'll ask you to address the conflicts- then I can give you a more formal review
I merged the addresses change. Also added styling for mobile and polished up the theme selector. Let me know if there are any other changes you'd like me to merge!
Nice work @charliemcgrady! Alll riiiight I took the code for a spin, and I have thoughts! Please understand I do not expect that my feedback here is conclusive/must be followed. I want to start conversation.
I propose that we call the pre-existing theme selector control (which now has types- thanks for that) the 'show/hide' or just 'show' or 'show type' control, and this new one can be the 'ActiveThemeDisplay' or 'ThemeSelector'.
What I love:
Potential changes:
The 'active' state prevents clicking into other themes. I think this is a little too restrictive- my vote is to allow selection of whatever, but if the user selects an element from a different theme, we should activate that theme in the new control.
So IF we allow clicking around willy-nilly, the user would get two types of feedback that 'stuff is going on': they'd see the active theme moving around from element to element in the new control, and they'd get the strong feedback of the actual map styling changing.
If we do that, maybe we change this new control from a 'control' to just an indicator- i.e. clicking on it doesn't do anything, it only updates when the user clicks a different item.
we can later add a control to do the disambiguation-on-click like we discussed during the summit (where a user clicks on a point, there are four map entities underneath it, and we give them a pop-up control to select which one they want to inspect).
Smol UI issues:
on screens of a certain size the old type selector control and the new 'active' theme selector overlap while the inspector panel is open (see highlighted area):
There seems to be some sort of issue with selecting a theme while the inspector panel is open- it's hard to reproduce but I found times where one or more themes aren't responding to selections- almost like an invisible div is hovering over them intercepting mouse events (cursor doesn't change, clicking does nothing).
I realize this is a lot of feedback, but I expect this is just the start to the conversation.
Some more thoughts and notes I had:
In reference to Ben's mention of not being able to click
into other themes, I think this presents an interesting feature that we could use. For instance, it allows me to do this: Highlight the Suffolk County division area, even when it is not the top layer. I am a fan of this, as it also lets the user click on transportation entities hidden below buildings. I can also see how it could be seen as restrictive, but that is why being to de-select a theme to be highlighted is even more important.
Next, I think the "selected" color indicator probably has to change now that
we are using a muted base layer. The road I have selected here is difficult to see:
I noticed this little funky interaction with the theme selector. It seemed more reproducible at lower window widths, so perhaps fixing the overlap issue Ben mentioned will also fix this. https://github.com/user-attachments/assets/4d94e12e-5fa4-49fc-bf3f-cf9e74ac649d
The last comment I had, and this might be too much detail, but I think with the expansion of the number of themes/types we have available to enable/disable, we should consider making each theme collapsible. This could also be pervasive between closing and opening the selector box. It would better allow the user to focus on a specific theme.
Last thing (I swear) is something that I just wanted to throw out since I have been thinking about it for a bit, and it seems most relevant now - would it be best to make the theme-selector a clickable dropdown with a close-button similar to the inspector panel? I think it could be valuable to allow users to explore the map with the theme-selector open to more dynamically switch between themes - @Bonkles thoughts?
Capturing some notes from our huddle here!
Further refinements (these can come later):
Open items