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

support nested filter specification types #757

Merged

Conversation

j8seangel
Copy link
Contributor

@j8seangel j8seangel commented Jan 5, 2022

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR. --> Support nested expressions and include suggestions for every filter type
  • Document any changes to public APIs.
  • Manually test the debug page.
  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

@HarelM
Copy link
Member

HarelM commented Jan 5, 2022

Thanks for taking the time to open this PR!
Note that you are editing a generated file. Please make sure to fix the code generation.
It would be great to let us know what are you solving.
Also it would be great if you can check the relevant tick boxes in this template.
If there's an issue you are solving with the PR, even better.

@j8seangel
Copy link
Contributor Author

Hi @HarelM First of all, big thanks for your effort!
I opened the PR by mistake because I wanted to test the changes in my fork first but once open I'll keep working on this one.

It would be great to let us know what are you solving.

We are moving from mapbox-gl and I found some definition errors on our existing expressions so I thought I could collaborate updating their types definition (thanks again for moving to typescript! 🙇 )

This was the error:
image

Writing expressions has been always difficult for me, so typing them are even harder but I think we can have a generic solution to allow writing any kind of expression. And that is why I added a list of every filter available in the documentation

Note that you are editing a generated file. Please make sure to fix the code generation.

I thought the src/style-spec/types.ts is a source type definition file, could you explain then a bit more how that works ?

Also it would be great if you can check the relevant tick boxes in this template.

Sure, let me accommodate and understand all and will keep you updated once they are ready

If there's an issue you are solving with the PR, even better.

I haven't found any, but If that is the workflow I can create one explaining the error and point it here

@j8seangel j8seangel force-pushed the support-nested-specification-types branch from 4f15f79 to 1435ba8 Compare January 5, 2022 15:00
@j8seangel j8seangel force-pushed the support-nested-specification-types branch from 145ae9d to ad251ab Compare January 5, 2022 15:04
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

Bundle size report:

Size Change: 0 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 185 kB 185 kB 0 B
maplibre-gl.css 9.49 kB 9.49 kB 0 B
ℹ️ View Details No major changes

CHANGELOG.md Outdated Show resolved Hide resolved
@HarelM
Copy link
Member

HarelM commented Jan 5, 2022

Please also fix the failing tests.
Thanks!! :-)

@j8seangel j8seangel force-pushed the support-nested-specification-types branch from 8622791 to b2c5a72 Compare January 6, 2022 11:06
@j8seangel
Copy link
Contributor Author

@HarelM all comments addressed, ready to review

@j8seangel j8seangel requested a review from HarelM January 6, 2022 11:15
@HarelM
Copy link
Member

HarelM commented Jan 6, 2022

Thanks!!!

@HarelM HarelM merged commit 39a0c70 into maplibre:main Jan 6, 2022
@j8seangel j8seangel deleted the support-nested-specification-types branch January 6, 2022 19:48
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.

None yet

2 participants