fix: support spaces as sperator for vendor-option graphic-margin #988
Conversation
| @@ -1001,7 +1001,7 @@ export class SldStyleParser implements StyleParser<string> { | |||
| if (this.withGeoServerVendorOption) { | |||
| const graphicFillPadding = getVendorOptionValue(sldSymbolizer, 'graphic-margin'); | |||
| if (!isNil(graphicFillPadding)) { | |||
| fillSymbolizer.graphicFillPadding = graphicFillPadding.split(',').map(numberExpression); | |||
| fillSymbolizer.graphicFillPadding = graphicFillPadding.split(/[,\s]/).map(numberExpression); | |||
There was a problem hiding this comment.
Great catch @sebastianfrey! Turns out GeoServer actually expects only spaces and no commas (validating the style does not show any errors, applying it however does). Could you adjust this accordingly?
There was a problem hiding this comment.
@jansule Adopted the write path for the graphic-margin prop. Is this the change you were looking for?
There was a problem hiding this comment.
Yes, perfect! Thanks for addressing the remarks @sebastianfrey
|
Waouh, thanks, I come across this issue right now. |
|
@jansule @sebastianfrey, Do we have a case where we should keep the comma separation notation ? I've seen only doc about value separated by spaces, and not comma. |
|
@ger-benjamin The question is: How did you come up with it? Did you find some sample SLD, which did use comma separated values? Or was it just by mistake? |
|
Ok, digged down the rabbit hole: GeoServer uses Geotools for SLD handling and Geotools only supports spaces for So it should be safe, remove the comma separated notation. |
|
I work with an older and new parser system. And the old one also added graphic-margin without comma. That's not a version problem or so. Can I open a PR right now to remove the comma support ? And again, sorry, for that... |
|
No worries @ger-benjamin, things like these happen. And thanks for catching the missing part in #989. I shouldn't do code reviews in the mornings 😄 |
Closes #987