StephanGeorg / staticmaps

A Node.js library for creating map images with markers, polylines, polygons and text.
MIT License
170 stars 49 forks source link

Added additional parameters to configure padding #58

Closed ghost closed 3 years ago

ghost commented 3 years ago

I have added two more paddings: inferior and superior, given that previously there was only the option to add one padding (that would work for both inferior and superior padding). The same with right and left padding.

StephanGeorg commented 3 years ago

Hi @marionajaramillo

Thank's for the PR. Maybe you could refactor the whole padding functionality to be more CSS style. One solution could be that there's only one padding option which expects an array where you can define all paddings like the CSS syntax.

/* Apply to all four sides */
padding: [10];

/* vertical | horizontal */
padding: [10, 20];

/* top | horizontal | bottom */
padding: [10, 20, 15];

/* top | right | bottom | left */
padding: [10, 20, 30 , 40]

I think it's more flexible and way more intuitive. What you think?

And would you please add a test? Thanks.

ghost commented 3 years ago

Thanks for the reply,

When I was trying to refactor the code, I saw that there was an error in my coding and it was more complicated than it seemed at first. That's why I've closed the pull request.

Thanks for maintaining staticmaps :)

Mariona Jaramillo Civill

El mar, 17 ago 2021 a las 15:19, Stephan Georg @.***>) escribió:

Hi @marionajaramillo https://github.com/marionajaramillo

Thank's for the PR. Maybe you could refactor the whole padding functionality to be more CSS style. One solution could be that there's only one padding option which expects an array where you can define all paddings like the CSS syntax https://developer.mozilla.org/en-US/docs/Web/CSS/padding#syntax.

/ Apply to all four sides / padding: [10]; / vertical | horizontal / padding: [10, 20]; / top | horizontal | bottom / padding: [10, 20, 15]; / top | right | bottom | left / padding: [10, 20, 30 , 40]

I think it's more flexible and way more intuitive. What you think?

And would you please add a test? Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/StephanGeorg/staticmaps/pull/58#issuecomment-900292277, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQV4PB3YXASVUADQTA3YLHDT5JOX3ANCNFSM5CJYTDCQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email .