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

Remove popup when marker is behind terrain #3865

Merged

Conversation

sbachinin
Copy link
Collaborator

Currently popups do not change opacity when hidden by terrain.
It looks especially bad when a popup is attached to a marker. Marker becomes semi-transparent, and popup doesn't.

By this PR I suggest to hide a "dependent" popup when its marker is behind terrain.
It's not the only possible solution but 1) it's simple and 2) it actually seems more adequate than making a popup semi-transparent because such semi-transparent popup will create a lot of visual noise.

So here I make sure that

  1. when marker gets hidden, it will close its popup
  2. when marker is hidden, click will not open its popup
popup.behind.terrain.before.after.mp4

This solution does not affect independent popups. They will still be fully opaque behind terrain.
It seems that visibility of dependent and independent popups are 2 different problems.
Visibility of a dependent popup should be determined by the marker.
Visibility of an independent popup should be determined by popup's own position relative to terrain.
It will require to somehow copy or reuse the bulky opacity-related code from marker.ts.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

src/ui/marker.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Mar 19, 2024

Looks like a reasonable UX, the code is basically a one line change, so I don't have real comments.

@codecov-commenter
Copy link

codecov-commenter commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 86.58%. Comparing base (44b582e) to head (1d987d2).
Report is 18 commits behind head on main.

Files Patch % Lines
src/ui/marker.ts 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3865      +/-   ##
==========================================
- Coverage   86.83%   86.58%   -0.25%     
==========================================
  Files         241      241              
  Lines       32341    32343       +2     
  Branches     1962     1981      +19     
==========================================
- Hits        28082    28003      -79     
- Misses       3340     3394      +54     
- Partials      919      946      +27     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented Mar 20, 2024

Also changelog is needed.

src/ui/marker.ts Outdated Show resolved Hide resolved
@HarelM
Copy link
Collaborator

HarelM commented Mar 21, 2024

Can you verify that the added scenario is covered in tests? According to the code coverage report it seems that it's not well covered...?

@HarelM
Copy link
Collaborator

HarelM commented Mar 21, 2024

THANKS!!

@HarelM HarelM merged commit e32f160 into maplibre:main Mar 21, 2024
15 checks passed
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.

3 participants