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

Staging to Master #462

Merged
merged 18 commits into from
Jun 29, 2022
Merged

Staging to Master #462

merged 18 commits into from
Jun 29, 2022

Conversation

vogelino
Copy link
Collaborator

This PR merges the PR #460 into Master and thereby releases the OSM Editor Link in the Tooltips.

@julizet
Copy link
Member

julizet commented Jun 26, 2022

Thanks for this really rigorous and even so quick review an PR @vogelino @dnsos @ff6347!

I've tested the deploy preview and found three things that could be improved – UX- and design-wise:

  1. it would be nice if one could close the sticky pump overlay. Once clicked it's there forever.
  2. when I clicked on a pump I see the sticky overlay. If I then change to the trees or rainfall layer, I can still see my stick pump overlay. Would be nice if it deisappears when the user changes the layer.
  3. the text-link to the editor should be aligned to the left

Please see the two screenshots attached and let me know what you think.

Tested on Catalina 10.15.4 with Firefox 99.0.1
462_changeLayer
462_stickyOverlay

@ff6347
Copy link
Member

ff6347 commented Jun 27, 2022

Good catch with changing the view type. The overlay disappears when moving the map but having a close button would be more explicit.

About the alignment: I actually like it on the right hand side. It's like you enter top left and leave bottom right.

@vogelino
Copy link
Collaborator Author

vogelino commented Jun 28, 2022

Hi @julizet and thx for your feedback! I agree with @ff6347 that the right alignment is well suited as last element on the tooltip. To be honest, this wasn't the reason why I did it though, much more it was because it was way easier to implement this way. Making it left aligned would require additional work.

I am now also closing the tooltip every time the mouse clicks outside of the tooltip. This solves every problem mentioned above. It makes the X obsolete I think. Here is the PR: #466

…p-refactor

Refactor/pump tooltip refactor
@vogelino
Copy link
Collaborator Author

vogelino commented Jun 28, 2022

@tordans the preview URL is this one: https://deploy-preview-462--giessdenkiez.netlify.app/

@julizet
Copy link
Member

julizet commented Jun 28, 2022

Hi @julizet and thx for your feedback! I agree with @ff6347 that the right alignment is well suited as last element on the tooltip. To be honest, this wasn't the reason why I did it though, much more it was because it was way easier to implement this way. Making it left aligned would require additional work.

I am now also closing the tooltip every time the mouse clicks outside of the tooltip. This solves every problem mentioned above. It makes the X obsolete I think. Here is the PR: #466

I agree with that -> clicking outside of the tooltip is a lightweight UX-solution. Let's skip this left-alignment in order to save dev capacities. If @tordans doesn't has anx further comments / feedback we are good to merge into main, IMHO

@vogelino
Copy link
Collaborator Author

Perfect @julizet. Thx for your feedback! I will merge it now.

@vogelino vogelino merged commit 4c7a024 into master Jun 29, 2022
@tordans
Copy link

tordans commented Jun 29, 2022

Looks great! Thanks! Will Twitter via @osmberlin once it is live. Will try adding clustering to the map later to improve the editing experience.

@vogelino
Copy link
Collaborator Author

It's live! 🟢

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

Successfully merging this pull request may close these issues.

None yet

6 participants