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

[bug] Map position should stay fixed when changing filters #466

Closed
tudoramariei opened this issue Dec 9, 2020 · 8 comments · Fixed by #763
Closed

[bug] Map position should stay fixed when changing filters #466

tudoramariei opened this issue Dec 9, 2020 · 8 comments · Fixed by #763
Assignees
Labels
bug Something isn't working client: ReactJS
Milestone

Comments

@tudoramariei
Copy link
Member

Currently, when a map filter is selected the map position changes to another place.

The map should stay fixed when changing filters and position should be changed only by the user by panning, zooming, and selecting a marker.

Current behaviour:
Screen Recording 2020-12-09 at 18 24 05 2020-12-09 18_27_18

@tudoramariei tudoramariei added bug Something isn't working map client: ReactJS labels Dec 9, 2020
@tudoramariei tudoramariei added this to the V1.0 milestone Dec 9, 2020
@GabrielMajeri
Copy link
Contributor

The reason why this is happening is because of these lines of code:

if (points.length > 1) {
const latitudes = points.map((p) => p.lat);
const longitudes = points.map((p) => p.lng);
const bounds = new H.geo.Rect(
Math.max(...latitudes),
Math.min(...longitudes),
Math.min(...latitudes),
Math.max(...longitudes),
);
currentMap.getViewModel().setLookAtData({ bounds, zoom: 10 }, true);
} else {
currentMap.getViewModel().setLookAtData({ zoom: 10 }, true);
}

They calculate the bounds of the new points, and tries to reposition the map so that it contains them all.

We could remove the code and this bug would be fixed, but are we sure we want to completely lose this behavior?

@tudoramariei
Copy link
Member Author

@GabrielMajeri sorry for the late response; I only just managed to test out the changes locally

I feel the current behaviour is flawed. That's because this feature does position the map at the centre of all the markers but it doesn't change the zoom level
e.g. if I add a building in Iasi, the starting position would be somewhere above Focsani and I can't really see any marker on the map 😒

It would be great if the map self-centred to show all markers when being opened for the first time and I would like a show me an overview of the map button
At the same time, I want the map to stay put when switching filters. If someone wants to see more details of a specific location they wouldn't want the map to just switch positions with every filter change

@tudoramariei
Copy link
Member Author

The PR I made is rather a POC. While not the best solution, it fixes the issue. Maybe we can find a better solution, though 🤔

@GabrielMajeri
Copy link
Contributor

The PR does solve this bug as is described. For your idea with a show me an overview of the map button, how is the button supposed to look like? We could add it as a regular HTML button, or implement it as a custom map UI button.

@tudoramariei
Copy link
Member Author

I'm waiting for some answers from UX to see if we actually do want a button. If not, we'll just kill the auto-centring feature of the map and call it a day.
So no sure answer here yet.

@tudoramariei tudoramariei added this to To do - Frontend in Seismic Risc Feb 4, 2021
@tudoramariei tudoramariei moved this from To do - Frontend to To do - Backend in Seismic Risc Mar 5, 2021
@myshy93
Copy link
Contributor

myshy93 commented Mar 15, 2021

why this is not closed?

@tudoramariei
Copy link
Member Author

Because it hasn't been fixed yet 😅
Closed it since I just didn't have the time to check how the old changes would've fit with the new map

@vladplesu
Copy link
Contributor

Hello, I can take this one if it`s still an issue.

@tudoramariei tudoramariei moved this from To do to In progress in Seismic Risc Apr 17, 2021
Seismic Risc automation moved this from In progress to Done Nov 25, 2021
tudoramariei pushed a commit that referenced this issue Nov 25, 2021
* replace scripts from head with here maps npm pkg
* create hook for map
* create map placeholder while loading
* update HereMapInteractive to use map hook
* update translations
* remove here maps npm package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment