Skip to content

Conversation

@shreeharshshinde
Copy link
Contributor

@shreeharshshinde shreeharshshinde commented Jul 5, 2025

Popup Padding Support

This PR introduces a new popupPadding option to the Popup class to address issue #5978.
The padding ensures that the popup remains within the visible area of the map by clamping its position based on user-defined or default padding values.


Key Changes

  • Added popupPadding to PopupOptions:

    popupPadding?: {
      top: number;
      bottom: number;
      left: number;
      right: number;
    };
  • Default padding values are set to :

      {
        top: 0,
        bottom: 0,
        left: 0,
        right: 0
      }
  • Example Usage

    const popup = new maplibregl.Popup({
     popupPadding: {
       top: 40,
       right: 60,
       bottom: 40,
       left: 60
     }
    })
    .setLngLat([lng, lat])
    .setHTML("<h1>Hello World</h1>")
    .addTo(map);

@HarelM
Copy link
Collaborator

HarelM commented Jul 5, 2025

Thanks for taking the time to open this PR.
How is this PR different than #6052?

Also note that creating a default that is different than current implantation is a breaking change and should not be made without proper planning of a breaking change version.
As a stop gap I would advise to keep it to 0 and suggest a breaking change feature when there are plans to create a breaking change version (also I don't think this breaking change is really needed)

@shreeharshshinde
Copy link
Contributor Author

Thanks for the feedback!
I’ve updated the default popupPadding to {top: 0, right: 0, bottom: 0, left: 0} so there’s no breaking change. Users can opt in by setting a custom value.
Let me know if there's anything else you'd like me to adjust!

Also, I personally prefer this approach as it stays minimal and intuitive without introducing additional API surface. Let me know if you'd like to move forward with this version or prefer closing in favor of #6052.

@HarelM
Copy link
Collaborator

HarelM commented Jul 6, 2025

I think that allowing to setPadding is needed to update the popup padding dynamically after the popup was created.
We can introduce this option first and then the other one if it is required, but I think it's a fuller solution.
In any case, tests are needed, also the CSS type is probably better, as I mentioned in the other PR.

@shreeharshshinde shreeharshshinde deleted the feat/popup-padding branch July 21, 2025 14:36
@shreeharshshinde shreeharshshinde restored the feat/popup-padding branch August 18, 2025 16:16
@HarelM
Copy link
Collaborator

HarelM commented Sep 5, 2025

@shreeharshshinde what's the status of this PR?

@HarelM
Copy link
Collaborator

HarelM commented Sep 10, 2025

How is this PR different than #6052?

(Besides the fact that the other PR is "stale")

@shreeharshshinde
Copy link
Contributor Author

The previous implementation mentioned in the feedback included:

  • A normalizePopupPadding function with robust input validation
  • Support for multiple input formats (number, PointLike, array, object)

However, the current implementation in the codebase uses a simpler approach:

  • Only object format with string values for padding
  • A setPadding method for dynamic updates
  • Direct application of CSS padding styles to the popup container

src/ui/popup.ts Outdated
* { top: '20px', right: '30px', bottom: '40px', left: '10px' }
* ```
*/
popupPadding?: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CSS style can have 1, 2 or 4 values, which is not the case here, I would recommend updating the docs or the type.

src/ui/popup.ts Outdated
this._container.style.maxWidth = this.options.maxWidth;
}
// Apply popupPadding as CSS
const padding = this.options.popupPadding;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part looks duplicated.

@shreeharshshinde
Copy link
Contributor Author

@HarelM I am not getting why still those build test are failing ? What is the reason behind it ?

@HarelM
Copy link
Collaborator

HarelM commented Sep 15, 2025

You need to update the build size to accommodate for the code changes.

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.

2 participants