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

Write GraphicFill for PolygonSymbolizer #388

Merged
merged 4 commits into from
Oct 4, 2021

Conversation

jansule
Copy link
Contributor

@jansule jansule commented Sep 3, 2021

Description

This introduces the support for writing GraphicFill as part of a FillSymbolizer.

The main work was done by @rdewit in #244. I mainly did some minor refactorings and added tests.

Also, we now set the CanvasPattern directly as the color for the fill property, instead of using a custom renderer. Thereby we eliminate the z-index issue.

Please note that the common FillSymbolizer properties will have no effect, when using GraphicFill. This however is in line with the behavior in GeoServer.

geostyler-ol-graphicFill

Related issues or pull requests

Replaces #244
Solves #240
Solves #228

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Dependency updates
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe)
  • I am unsure (we'll look into it together)

Do you introduce a breaking change?

  • Yes
  • No
  • I am unsure (no worries, we'll find out)

Checklist

  • I understand and agree that the changes in this PR will be licensed under the BSD 2-Clause License
  • I have followed the guidelines for contributing
  • The proposed change fits to the content of the code of conduct
  • I have added or updated tests and documentation, and the test suite passes (run npm test locally)
  • I'm lost; why do I have to check so many boxes? Please help!

jest.config.js Outdated Show resolved Hide resolved
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.

Nice work. Not sure if you want to resolve the commented TODOs before merging.
Please also check if geostyler-style still has to be transformed for jest.

src/OlStyleParser.ts Outdated Show resolved Hide resolved
src/OlStyleParser.ts Outdated Show resolved Hide resolved
@jansule
Copy link
Contributor Author

jansule commented Oct 3, 2021

Thanks for the reviews @KaiVolland @nevaldas!

Regarding the TODOs: I would rather keep them for now, as we have to check if we actually run into performance issues somewhere. For now, I would say it is ok as is.

@nevaldas
Copy link
Contributor

nevaldas commented Oct 4, 2021

We are using this already in our solution (yeah, we needed this ASAP). And the issue we had was that if image is not loaded when this code is executing, the getOlPatternFromGraphicFill returns null, meaning that image is not shown until map is refreshed. That happens when user moves the map, changes zoom level or can be forced via JS. This might be inconvenient. But in our case - I modified that line to check until image has loaded and force refresh of map (via JS). Sadly - that's not a global solution I could write here, as it requires access to the map class, which is not available from this part of code.

@KaiVolland
Copy link
Contributor

Moved the discussion to issue #434

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.

3 participants