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

Map Data checkbox: perhaps use toggle slider instead #5009

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nertc
Copy link
Contributor

@nertc nertc commented Jul 20, 2024

This PR addresses "Map Data checkbox: perhaps use toggle slider instead" issue mentioned in the #4931

Several changes were made to the data loading functionality:

  1. Clicking "Map Data" checkbox will add loading spinner to the label. It will be shown until back end collects and returns all the information (see "Loading Video"). After all the information is returned, spinner will be removed.
  2. After getting information and agreeing to show it on the map, rendering process will be asynchronous. Therefore, there will be no more browser or page freezes and it will have animation like visual (see "Render Video").
  3. If user unchecks "Map Data" checkbox, removing process will be also asynchronous and there won't be any freezes. It will also have animation like visual (see "Remove Video").
  • Checkbox state will be updated as soon as it is clicked, so changing it to the toggle one has no more benefits.
  • If the user decides to uncheck "Map Data" while back end collects information, loading spinner will disappear.
  • If the user decides to uncheck "Map Data" while objects is rendered, it won't trigger change until the rendering process is done. After the process, application will check and if "Map Data" is unchecked, it will start removing rendered objects.

Connected to the PR openstreetmap/leaflet-osm#38
Fixes #4931

Videos:

Loading Video

OpenStreetMap.and.33.more.pages.-.Work.-.Microsoft.Edge.2024-07-25.11-51-38.mp4

Render Video

OpenStreetMap.and.28.more.pages.-.Work.-.Microsoft.Edge.2024-07-20.16-55-01.mp4

Remove Video

OpenStreetMap.and.28.more.pages.-.Work.-.Microsoft.Edge.2024-07-20.16-56-42.mp4

@nertc nertc force-pushed the issues_4931_map_data_checkbox branch 4 times, most recently from 5f00a0b to 0e1677a Compare July 20, 2024 13:48
@AntonKhorev
Copy link
Collaborator

Are you planning to get the changes to vendor/assets/leaflet/leaflet.osm.js merged in https://github.com/openstreetmap/leaflet-osm ?

@nertc
Copy link
Contributor Author

nertc commented Jul 22, 2024

@AntonKhorev Yes. Should I create a separate PR in the https://github.com/openstreetmap/leaflet-osm for it?

@nertc nertc force-pushed the issues_4931_map_data_checkbox branch 2 times, most recently from d07bf66 to d9615c6 Compare July 24, 2024 16:03
@nertc
Copy link
Contributor Author

nertc commented Jul 24, 2024

I created a PR for the leaflet-osm (openstreetmap/leaflet-osm#38) that needs to be merged before this PR will be, as asynchronous loading part depends on it.

@nertc nertc marked this pull request as ready for review July 24, 2024 16:16
@nertc nertc force-pushed the issues_4931_map_data_checkbox branch from d9615c6 to 41e6069 Compare July 25, 2024 07:45
@nertc
Copy link
Contributor Author

nertc commented Sep 3, 2024

This PR is linked to the openstreetmap/leaflet-osm#38. If leaflet-osm PR is not merged, this PR can't be merged.

@nertc nertc force-pushed the issues_4931_map_data_checkbox branch from 41e6069 to ca019cd Compare September 23, 2024 08:39
@nertc
Copy link
Contributor Author

nertc commented Sep 23, 2024

PR was updated to include changes made in the openstreetmap/leaflet-osm#38. As leaflet-osm PR is now merged, this PR can be reviewed and merged.

@AntonKhorev
Copy link
Collaborator

Now you can check Map Data, wait for the rendering to start then quickly click Map Data 2*n times and watch how data is added then removed then added then removed... n times. While this is happening and Map Data is checked, pan to a different place to get a stream of javascript errors.

@nertc nertc force-pushed the issues_4931_map_data_checkbox branch from ca019cd to 4b91e79 Compare October 1, 2024 08:20
@nertc
Copy link
Contributor Author

nertc commented Oct 1, 2024

@AntonKhorev I couldn't reproduce JS errors in the console. But I changed logic and now when user clicks "Map Data" checkbox, previous action is skipped immediately. Therefore, there will be no 2*n loads and errors. When we will be okay with the solution, I'll create a new PR in the leaflet-osm.

@nertc
Copy link
Contributor Author

nertc commented Oct 23, 2024

If there are no other change requests for this PR, I'll create a new PR for the leaflet-osm to merge changes done in the vendor/assets/leaflet/leaflet.osm.js.

@nertc nertc force-pushed the issues_4931_map_data_checkbox branch from 4b91e79 to af830a1 Compare February 4, 2025 09:13
@nertc nertc force-pushed the issues_4931_map_data_checkbox branch from af830a1 to fca12a2 Compare February 4, 2025 11:18
@nertc
Copy link
Contributor Author

nertc commented Feb 4, 2025

PR was updated. Branch was synced with master and conflicts were resolved. Also, instead of handling canceling layer loading in the setTimeout callback, now it is handled by clearing setTimeouts.

Optimizations for layer rendering were done in openstreetmap/leaflet-osm#43, but still this PR is relevant, because it adds loading spinner for those cases when data is processed by the back end, and also for slower CPUs (for example, when I set 6x slowdown to the CPU) it will provide smooth asynchronous loading without any freezes.

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.

Map Data checkbox: perhaps use toggle slider instead
3 participants