-
-
Notifications
You must be signed in to change notification settings - Fork 932
Implement data-driven styling for line-dasharray #5812
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
Conversation
This would be a good use case for being able to form arrays dynamically: maplibre/maplibre-style-spec#950. |
…riven-dasharray # Conflicts: # package-lock.json # package.json
1f161aa to
b7ae6b9
Compare
|
Can you fix the lint warning please? |
This reverts commit d382706.
|
I've done another round of code review changes. Again, I've left open any conversations that I wasn't able to obviously resolve. |
| path: coverage-merged | ||
|
|
||
| - name: Code Coverage Annotation | ||
| if: github.event.pull_request.head.repo.full_name == github.repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably the least problematic one as if all lines are covered this wouldn't fail, are you sure this creates problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that removing this if: condition in d382706 caused this step to fail.
- Our code coverage reporting and enforcement step continues to work correctly.
- The failure appears to be related to annotation permissions. Since this workflow is running from my fork, it doesn't have the necessary permissions to create annotations in the main MapLibre GL JS repository. The annotations are helpful but disabling them does not compromise our quality standards.
- Regarding the four statements marked as uncovered: these are actually covered by our render tests. I verified this by commenting them out and watching the render tests fail. This type of coverage reporting discrepancy is something we've seen before with our test setup. Addressing it would be outside this PR's scope.
Based on these factors, keeping the if: condition seems like the right approach here.
HarelM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll go ahead and release the spec package.
|
I've just released version 24.2.0 of the style-spec. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
I've done another round of revisions, including updating |
|
Is there a demo using this new feature anywhere? |
I don't have anything up online. Sorry! |
|
I think the only thing left is the changelog. Can you please add to it and I'll merge this PR and release a new version. |
…gl-js into data-driven-dasharray
|
Done |
See also maplibre/maplibre-style-spec#1100
This PR resolves #1235 by making the
line-dasharrayproperty "data-driven", meaning it can now reference feature property values in expressions.Due to limits in the expression system, dasharrays must be specified in the style using the
["literal", [...]]syntax. Feature properties cannot directly contain array values.Example:
Benchmarks
See full results at all.pdf
Launch Checklist
Phase 1: Basic Functionality
line_sdf.vertex.glslandline_sdf.fragment.glslline_program.tsprogram_configuration.tsline-dasharrayand its associatedvaryings underattributeNameExceptionsCrossFadedConstantBinderfor line-dasharray supportProgramConfigurationfor line-dasharray supportstyle/properties.tsCrossFadedDataDrivenPropertyto ensure support for data-drivenline-dasharraydraw_line.tsprogramConfiguration.setConstantDasharrayforline-dasharrayInclude before/after visuals or gifs if this PR includes visual changes.lineDasharrayAttributeswithpatternAttributesas support for the latter is better throughout the codebaseLineAtlas#getDashPhase 2: Pre-Code Review
dash_attributes.tsCHANGELOG.mdunder the## mainsection.Phase 3: Code Review
Underway
"interpolate"expressions with arrays. (link)nullin one expression. (link)global-stateexpressions with array literals. (link)Phase 4: Post Code Review
See #5768 for similar workflow
postinstallscript hack and updatepackage.jsonto reference latest version ofmaplibre-style-spec