search

maplibre / maplibre-style-spec

MapLibre Style Specification & Utilities
https://maplibre.org/maplibre-style-spec/
Other
93 stars 65 forks source link

Remove interpolate-projection #901

Closed HarelM closed 2 weeks ago

HarelM commented 2 weeks ago

Launch Checklist

@birkskyum I took a stab at using the interpolate for projection instead of introducing a new interpolation expression.

I'm still looking into understanding how color names are being parsed, but in general, I think this looks like a good direction. Let me know what you think.

codecov-commenter commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (107bd0e) to head (72bf603).

Files with missing lines Patch % Lines
src/expression/index.ts 60.00% 2 Missing :warning:
src/util/projection.ts 77.77% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## projection-expression-collecting-branch #901 +/- ## =========================================================================== - Coverage 92.63% 92.56% -0.07% =========================================================================== Files 107 107 Lines 4749 4761 +12 Branches 1346 1352 +6 =========================================================================== + Hits 4399 4407 +8 - Misses 350 354 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

HarelM commented 2 weeks ago

Edit: I think it is working as expected now according to the tests.

birkskyum commented 2 weeks ago

I like it a lot! Nice that it's going away the interpolate-projection.

If we in any way can find a way to have just [step, [zoom], number, string, number, string] I think it would be worthwhile.

I imagine it's possible right now to do:

[step, [zoom], number, Projection, number, Projection]

... and then have a Projection.parse("mercator") ? It's not too bad, considering it's not repeated, but I'm thinking there was to be a way we can make this work [number, string, number, string].

HarelM commented 2 weeks ago

I can merge this to the other branch and you could test this. I think it should work, shouldn't it?

HarelM commented 2 weeks ago

[number, string, number, string] is not an expression that is supported and I don't want to invent more expressions. So I think this is a good middle ground.

HarelM commented 2 weeks ago

Feel free to merge when you think it's OK.