Add max width for popups #7906
Add max width for popups #7906
Conversation
| @@ -231,6 +234,22 @@ export default class Popup extends Evented { | |||
| return this.setDOMContent(frag); | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
This has to be moved above the setMaxWidth (or set maxWidth if you go the getter/setter route) method. You should also add a doc comment for getMaxWidth.
src/ui/popup.js
Outdated
| /** | ||
| * Sets the popup's max width. | ||
| * | ||
| * @param pixel A string representing the pixel value for the maximum width. |
There was a problem hiding this comment.
The name of the parameter here in the doc should be the same as the argument to the method, which in this case is maxWidth. I'd also make it clear that this is setting the CSS property maxWidth and clarify that the expected string format is "Npx" where N is some number.
src/ui/popup.js
Outdated
| return this._container; | ||
| } | ||
|
|
||
| setMaxWidth(maxWidth: string) { // should this be number? |
There was a problem hiding this comment.
I think it makes sense to keep this as a string since it's setting a CSS property which takes a string. I could envision taking a number as input and converting it to a string but this property doesn't have to be in pixels. It could be ems, rems, points or any other valid CSS unit of measure and we wouldn't know what the user intended if they supplied a number. It may be reasonable to default to pixels, but I think it's clearer to be explicit and let the user define the units as part of the input.
src/ui/popup.js
Outdated
| setMaxWidth(maxWidth: string) { // should this be number? | ||
| this.options.maxWidth = maxWidth; | ||
| this._update(); | ||
| return this; |
There was a problem hiding this comment.
If you convert this to a setter method, there's no reason to return this at the end (because it won't be a chainable method anymore).
mzdraper
force-pushed
the
popup-max-width
branch
from
9699032
to
392c9aa
Compare
|
@mzdraper there's a Flow error that you'll have to fix before you can merge |
* release-liquid: (84 commits) v0.54.0 (take two) (#8184) Fix disappearing controls in Safari 12+ (#8193) (#8194) [docs] token refactor in docs-page-shell (#8174) (#8181) v0.54.0-beta.2 (#8166) Bugfix - removeFeatureState fails with target.id === 0 (#8150) (#8164) update one more frame after canvas source paused (#8130) (#8163) move docs dependencies to dev (#8121) (#8129) v0.54.0 (#8115) CP removeFeatureState fix (#8090) v0.54.0-beta.1 (#8084) Allow setStyle diff by default with localIdeographFontFamily on (#8081) Upgrade various (mostly dev) deps, downgrade GL (#8078) Fix default marker positioning (#8074) Use @mapbox/gazetteer for benchmark coordinates (#8063) bump react-dom to v16.3.3 to fix minor vulnerability warning Fix and refactor the benchmark suite (#8066) Add max width for popups (#7906) Extrusions: Do not try to triangulate non-polygon type features (#7685) docs fixes after the merge limit flow max_workers and upgrade to v0.95.1 (#8061) ...
|
I think it could be achieved by setting a class to new mapboxgl.Popup({
...
className: 'some-class'
})...And then you could customize this class on your CSS as .some-class {
max-width: 240px;
}Just updated Mapbox and it breaks a lot my map. Now you enforce some I would like to updated Mapbox to get all the benefits from it without such a burden as it covers only a specific case where there is a nice and supported workaround already. I had to add |
The accepted convention on semver is that 0.x versions may introduce breaking changes with a minor version change. This was released as a part of v0.54.0 and documented as a breaking change. So we did this right. |
Sorry @mourner my bad, I was with 1.0 release in mind. Nevertheless I don't want to focus on it here. My main input is regarding the feature released that breaks stuffs and I think it doesn't add much to the library as there was another solution for the very same problem already implemented. |
Launch Checklist
This adds a max width parameter for popups.
Closes #1862