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

Add isZoomExpression #334

Merged
merged 1 commit into from
Sep 12, 2023
Merged

Conversation

tangerine-orange
Copy link
Contributor

@tangerine-orange tangerine-orange commented Sep 12, 2023

Addresses #267

In maplibre/maplibre-gl-js#2837, we need a way of checking if a given expression is a ZoomConstantExpression, and then casting it as such. In this PR, we expose an isZoomExpression type guard.

Note: I'm not sure if I'm meant to post a PR where I integrate a staged version of maplibre-style-spec into maplibre-gl-gs?

Launch Checklist

  • [x ] 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.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

Copy link
Collaborator

@HarelM HarelM left a comment

Choose a reason for hiding this comment

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

This is generally approved, please check that it does what you would expect when using it in gl-js.

@HarelM
Copy link
Collaborator

HarelM commented Sep 12, 2023

I now read the linked issue comment saying you tested it to work as expect, cool, thanks!
Please create a version bump PR after this is merged so I can release a new version of style spec that will address the "real" issue.

@HarelM HarelM merged commit 594e98d into maplibre:main Sep 12, 2023
6 checks passed
@HarelM
Copy link
Collaborator

HarelM commented Sep 12, 2023

Oh, a changelog entry is missing, can you add it as part of the version bump?

@tangerine-orange
Copy link
Contributor Author

Oh, a changelog entry is missing, can you add it as part of the version bump?

🤦 Ugh, I had a few local changes that I didn't commit 🤦, including the changelog entry, and those changes were what I was testing mapbox-gl-js against. I added both to #335.

@tangerine-orange
Copy link
Contributor Author

Oh, a changelog entry is missing, can you add it as part of the version bump?

🤦 Ugh, I had a few local changes that I didn't commit 🤦, including the changelog entry, and those changes were what I was testing mapbox-gl-js against. I added both to #335.

I guess best practice is to use the pushed branch version of mapbox-style-spec to test, instead of using the local copy like I did.

@HarelM
Copy link
Collaborator

HarelM commented Sep 13, 2023

It is, but it's a chicken and egg problem as we wouldn't want to merge things that we're not sure are working...

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.

2 participants