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

feat: add vendor-option graphic-margin #971

Merged

Conversation

ger-benjamin
Copy link
Member

@ger-benjamin ger-benjamin commented Nov 18, 2024

For #969

I add here a first VendorOption with the graphic-margin property. This is linked to the
additional Geostyler-styler GraphicFillPadding property: geostyler/geostyler-style#651

As It's the first VendorOption, I've several questions:

  1. Is the code looking as expected ?
  2. Can I add tests directly into src/SldStyleParser.geoserver.spec.ts ? Or should I had a dedicated test file ?
  3. How can we add this new withVendorOption parameter into the Geostyler demo, cli or qgis plugin ? Should I create and issue about that ("be more flexible with parser options") ?

@ger-benjamin ger-benjamin force-pushed the Add-first-vendor-option-graphic-margin branch 2 times, most recently from 2fd05bc to 94b1167 Compare December 3, 2024 13:20
@jansule
Copy link
Contributor

jansule commented Jan 7, 2025

Is the code looking as expected ?

Basically it looks good. Maybe it is smarter to have the vendorOptions deactivated by default.

Can I add tests directly into src/SldStyleParser.geoserver.spec.ts ? Or should I had a dedicated test file ?

Adding them to the src/SldStyleParser.geoserver.spec.ts tests should be fine. (I forgot that there already exists a geoserver spec file in the call today...)

How can we add this new withVendorOption parameter into the Geostyler demo, cli or qgis plugin ? Should I create and issue about that ("be more flexible with parser options") ?

Best way would probably to add an additional option just for GeoServer SLD, so users can decide if they want to use official SLD or GeoServer SLD. E.g. the format dropdown of the GeoStyler Demo should show multiple SLDs formats (SLD 1, SLD 1.1, GeoServer SLD 1, GeoServer SLD 1.1).

@ger-benjamin ger-benjamin force-pushed the Add-first-vendor-option-graphic-margin branch from 94b1167 to e9555ad Compare January 11, 2025 14:10
@ger-benjamin ger-benjamin force-pushed the Add-first-vendor-option-graphic-margin branch from e9555ad to 19e53d0 Compare January 13, 2025 07:37
@ger-benjamin ger-benjamin force-pushed the Add-first-vendor-option-graphic-margin branch from 19e53d0 to 3429c06 Compare January 13, 2025 07:41
@ger-benjamin ger-benjamin marked this pull request as ready for review January 13, 2025 09:22
@ger-benjamin
Copy link
Member Author

ger-benjamin commented Jan 13, 2025

Thanks for the answers.
I've updated this PR, now it should works fine and is ready for a review.

Copy link
Contributor

@KaiVolland KaiVolland left a comment

Choose a reason for hiding this comment

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

Very nice. Thanks

@ger-benjamin
Copy link
Member Author

Thanks for the review @KaiVolland (I' can 't merge by myself)

@KaiVolland KaiVolland merged commit be950ea into geostyler:main Jan 13, 2025
6 checks passed
github-actions bot pushed a commit that referenced this pull request Jan 13, 2025
## [7.1.0](v7.0.0...v7.1.0) (2025-01-13)

### Features

* add vendor-option graphic-margin ([#971](#971)) ([be950ea](be950ea))

### Bug Fixes

* update image format handling ([0093c6b](0093c6b))
@GeoStyler-BOT
Copy link

🎉 This PR is included in version 7.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants