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

Location selector: Move location on map click #22198

Merged

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Oct 2, 2024

Proposed change

Add an icon-button to ha-selector-location which drops the marker at the center of the current viewport.

Move marker on click for location selector.

This is much faster than the old method (which I find very cumbersome), where you have to:

  • zoom out so you can see the current marker location and target location
  • drag the marker to the roughly desired area
  • zoom in
  • refine marker location
  • repeat zoom/refine

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue or discussion:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

This comment was marked as off-topic.

@MindFreeze MindFreeze added the Needs UX Pull requests requiring a review from the Home Assistant design team label Oct 29, 2024
@MindFreeze
Copy link
Contributor

Thanks for bringing this up. The team discussed this and we decided a better UX would be to move the marker on single press. So you tap/click the map anywhere to place the marker there. Obviously this shouldn't interfere with panning the map or resizing the circle.
Are you interested in implementing this @karwosts ?

@MindFreeze MindFreeze removed the Needs UX Pull requests requiring a review from the Home Assistant design team label Oct 30, 2024
@karwosts
Copy link
Contributor Author

I poked at this for a bit but so far I've been unsuccessful in figuring out how to generate a click event on the base map. I may come back to it to try again sometime but I'm not sure.

Right now the map shows the grab cursor on hover, would it be changed to the pointer cursor instead?

@MindFreeze
Copy link
Contributor

Right now the map shows the grab cursor on hover, would it be changed to the pointer cursor instead?

@matthiasdebaat what do you think?

@karwosts karwosts added the help-wanted In need of Additional Help label Oct 30, 2024
@matthiasdebaat
Copy link
Collaborator

Right now the map shows the grab cursor on hover, would it be changed to the pointer cursor instead?

Yes, I think this is needed to make this interaction work. On drag it should switch to the grab cursor.

We have to check if this also works on mobile. Alternative for touch interfaces could be to have the marker in a fixed center position of the map, allowing users to drag the map underneath instead of dragging the marker itself.

@wendevlin
Copy link
Contributor

Hi @karwosts,

you can add event listeners to the leaflet map object in ha-map.ts: https://leafletjs.com/reference.html#map-click

this.leafletMap.on("click", () => { fireEvent(this, "click") })

then you can handle the click somewhere.
And leaflet will handle that it's not a drag or something else.

@karwosts
Copy link
Contributor Author

karwosts commented Dec 3, 2024

Ok I've changed the approach as requested. Removed the new button, and now inside the ha-selector-location, clicking on the map moves the target to the location of the click.

It also seemed to work as expected on my Android mobile as well (tap moves the marker, hold pans the map).

mapclick

@karwosts karwosts removed the help-wanted In need of Additional Help label Dec 3, 2024
@karwosts karwosts changed the title Add button to location-selector to move marker to current view Location selector: Move location on map click Dec 3, 2024
@wendevlin wendevlin added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Dec 3, 2024
@wendevlin
Copy link
Contributor

Great thanks, I can test it tomorrow on my iPhone.

MindFreeze
MindFreeze previously approved these changes Dec 4, 2024
@MindFreeze
Copy link
Contributor

LGTM. Will leave it for @wendevlin to merge after trying iOS

src/components/map/ha-locations-editor.ts Outdated Show resolved Hide resolved
src/components/map/ha-locations-editor.ts Outdated Show resolved Hide resolved
src/components/map/ha-locations-editor.ts Outdated Show resolved Hide resolved
src/components/map/ha-map.ts Show resolved Hide resolved
@home-assistant home-assistant bot marked this pull request as draft December 4, 2024 07:26
@home-assistant
Copy link

home-assistant bot commented Dec 4, 2024

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@wendevlin
Copy link
Contributor

Great thanks, I can test it tomorrow on my iPhone.

iOS works fine ✔️

@karwosts karwosts marked this pull request as ready for review December 4, 2024 14:26
@home-assistant home-assistant bot requested a review from wendevlin December 4, 2024 14:26
@karwosts
Copy link
Contributor Author

karwosts commented Dec 4, 2024

Added double-click handling.

@MindFreeze MindFreeze dismissed wendevlin’s stale review December 5, 2024 06:55

requested changes have been addressed

@MindFreeze MindFreeze merged commit a3ca889 into home-assistant:dev Dec 5, 2024
15 checks passed
@karwosts karwosts deleted the location-selector-snap-current branch December 5, 2024 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hacktoberfest Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants