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

Introduce padding for popup #12708

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

sienki-jenki
Copy link

Hey, In my app there are HTML elements overlayed on top of map container - Popup of course does not respect them and hides below them.
In this PR I introduced optional padding for Popup class so popup anchor is not calculated only based on map container dimensions but also takes given padding into consideration.

Before:

popup-without-padding.mov

After:

popup-with-padding.mov

@mapbox

Launch Checklist

  • briefly describe the changes in this PR
  • 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
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Introduce padding to Popup</changelog>

@sienki-jenki sienki-jenki requested a review from a team as a code owner May 11, 2023 16:14
@CLAassistant
Copy link

CLAassistant commented May 11, 2023

CLA assistant check
All committers have signed the CLA.

@sienki-jenki
Copy link
Author

Hey @mourner, is there a chance that this PR will be reviewed by someone from mapbox?

@rreusser
Copy link
Contributor

rreusser commented May 22, 2023

Nice, thanks for the contribution @sienki-jenki! 👏 🙇 This seems reasonable to me. Just checking, how do you think this compares to modifying popups to respect global map padding (which affects the map vanishing point) instead of adding separate padding for the popup specifically? The advantage of your approach is maximum configurability; the advantage of the alternative is to improve popup behavior even for people who don't specifically opt-in and also falls gently on the side of avoiding an ever-growing list of parameters where they can be avoided.

Does this assumption seem reasonable, that necessary behavior wouldn't be lost if global padding and popup padding avoidance were one and the same?

Just to be certain, I double checked and it seems this does not currently nudge the popup to avoid padding:

map.once('load', () => {
    map.jumpTo({padding: {top: 100, right: 100, left: 100, bottom: 100}})
    map.showPadding = true
})

@sienki-jenki
Copy link
Author

Hey @rreusser, thanks for suggestion. Yes that makes sense, I've applied changes regarding padding - but I also noticed one flaw in current logic that I patched up, to be exact I'm talking about behaviour of anchor when popup is on either side of the screen. Even when anchor was left or right full height of the container was taken into consideration instead of half, which in my opinion led to waste of space as popup changed it's anchor too quickly, hope that video below explains it entirely.

Before:

before.mov

After:

after.mov

@sienki-jenki
Copy link
Author

@rreusser Actually in such way that map.setPadding() works does not fix my problem. When calling map.setPadding() map is being moved instantly - I don't want to move map at all, I just want to change padding to prepare map for movement that may/may not occur, so my changes are pointless here as I wouldn't be able to use it :D.
Maybe I'm missing something and there is a way to setPadding without map movement?

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

Successfully merging this pull request may close these issues.

None yet

4 participants