-
Notifications
You must be signed in to change notification settings - Fork 59
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
Openlayers types #1959
Openlayers types #1959
Conversation
One thing that should get checked, are the attributions. But this is unrelated to this PR, see #1962 I used the public function I fixed the types of the For the type ArrayTwoOrMore I introduced a type guard Another new type guard is |
I reverted the changes related to the selects. |
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.
Nice one. Ol types would be very cool if they are up to date.
Please have a look the import statements to have the Ol
prefix.
Even if the changes are mostly about typing i think they introduce api changes, so we have to release a major version (which is not negative).
One thing about actuality of the types: They should change only in details on minor version changes of openlayers. The hanreev types base for now on ol On the next major version jump we would need to look at it again, but I am working on type improvements in the openlayers jsdoc types that will make the auto generated types usable in our projects. The hanreev types are way better, as they provide more details, for example they type the events that will be passed to the listeners. |
29bc382
to
cac1860
Compare
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.
Nice work @simonseyock .
src/HigherOrderComponent/TimeLayerAware/TimeLayerAware.spec.tsx
Outdated
Show resolved
Hide resolved
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.
Nice one, thanks for this.
I've added some notes which you maybe want to address.
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.
Very nice @simonseyock, I just addressed some minor issues regarding our current naming conventions and a note about moving code to separate packages.
src/HigherOrderComponent/TimeLayerAware/TimeLayerAware.spec.tsx
Outdated
Show resolved
Hide resolved
257d1fc
to
9b3acd9
Compare
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 for the productive discussion and your effort in this. Feel free to merge once the ci is happy.
FEATURE | BUGFIX
This PR introduces the
@hanreev/ol-types
package and fixes all openlayers typing errors.