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

Version 5.44.0 #21

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Version 5.44.0 #21

wants to merge 8 commits into from

Conversation

0bmxa
Copy link

@0bmxa 0bmxa commented Jun 8, 2020

Updated with changes for v5.44.0.

I'm not very experienced with typescript and typings, so I essentially copied how the other typings looked. npm run test and npm run lint run without errors.

I had a few issues, where I'm curios how you'd have done that:

  • many methods/properties had no description available in Apple's docs, so I didn't know what to add here. Would you prefer to make them up or just leave them empty?
  • setCameraBoundaryAnimated accepts two different types as its first parameter, but according to Apple's docs they use a different parameter name depending on the type (coordinateRegion: mapkit.CoordinateRegion | mapRect: mapkit.MapRect), which (as far as I understand – please correct me if I'm wrong) is not supported in typescript. I tried overloading the method, i.e. adding it twice (one for each parameter name), but the linter insisted on combining them, and I wasn't sure if silencing the linter is more desirable, so I changed it.
    See the docs here and the line in my code here.
  • I also noticed, several properties being marked as optional. I also did that where test would fail because of them not being optional, but I didn't really understand where that is appropriate. Should that be done everywhere?

I'm happy about feedback, or also happy to change anything, just let me know!

@wsmd
Copy link
Owner

wsmd commented Jun 8, 2020

Thanks for the PR @0bmxa! I'll take a look and get back to you very soon!

@treemmett
Copy link

@wsmd Any chance this could get a review?

@andreicocari
Copy link

Bump?

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

4 participants