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

OpenLayers 5 #183

Merged
merged 1 commit into from
Apr 9, 2019
Merged

OpenLayers 5 #183

merged 1 commit into from
Apr 9, 2019

Conversation

samuel-girard
Copy link

Hi,
I'm working on the integration with OpenLayers 5.
All the work will be done on this branch.

Hope to have something to show within the next few days.

@davinkevin
Copy link
Contributor

Can you rebase it on the branch next ?

@samuel-girard
Copy link
Author

Hello
I just rebased it on next branch

@davinkevin
Copy link
Contributor

I still have the change done by @Neonox31 in the pull request which is not coherent with a rebase upon next branches. Could you check it?

BTW, I've seen a lot of style modification in your code (mainly around the @input on two line instead of one). Could you follow what has already be done to simplify the merge request. As is, it seems to be too many modification to still be clear.

Thanks

@samuel-girard samuel-girard changed the base branch from master to next September 26, 2018 21:35
@samuel-girard
Copy link
Author

OK, I previously only rebased the branch, but not the PR.
It is OK now, you shouldn't see any more of these style modifications.

For information, this is not yet ready to be merged: it is working well as it is (no issue found so far), but it lacks of OpenLayers typings. I temporarily created an ol-models to workaround this problem.
There is a big work beeing done in OpenLayers for that, so I hopefully can fix this soon.

@FallenRiteMonk
Copy link
Contributor

what is the status of this?

@samuel-girard
Copy link
Author

Hello,
Updated after the release of OpenLayers 5.3, this branch can now be merged.
There are still typings issues: for example your IDE will not offer you autocompletion for OpenLayers types used inside your application, but at least, ngx-openlayers will not fail to compile.

There is a workaround for the autocompletion if it is a big issue: openlayers/openlayers#8448 (comment)

@samuel-girard
Copy link
Author

Hi @Neonox31
I rebased this branch on next. You can merge it.
Thanks

@HarelM
Copy link
Contributor

HarelM commented Feb 3, 2019

Any updates on this? It would really help to have ol 5 in next branch and pre-released to npm.

@Neonox31 Neonox31 force-pushed the next branch 2 times, most recently from 20086b9 to ee186a8 Compare February 4, 2019 10:43
@Yakoust
Copy link
Contributor

Yakoust commented Mar 26, 2019

@kekel87 @davinkevin @Neonox31 @dabbid could you please have a look on this PR?

@kekel87
Copy link
Contributor

kekel87 commented Mar 26, 2019

There are still openlayers imports in attribution.component.ts and attributions.component.ts.

@Yakoust
Copy link
Contributor

Yakoust commented Apr 3, 2019

@samuel-girard are you available to debate/resolve discussions?

@samuel-girard
Copy link
Author

Hello @Yakoust
Thanks for your feedback on this PR.
I quickly had a look at your points and it seems good to me, although I haven't checked everything in details.
Unfortunately, I will not have time to apply the changes so I will be thankful if someone here could take care of it.

@Yakoust Yakoust force-pushed the openlayers_five branch 2 times, most recently from cde5b0b to 56a0a28 Compare April 4, 2019 13:40
@Yakoust
Copy link
Contributor

Yakoust commented Apr 4, 2019

There are still openlayers imports in attribution.component.ts and attributions.component.ts.

I also export those components in module. There were missing

@Yakoust
Copy link
Contributor

Yakoust commented Apr 8, 2019

Thanks @remiHau for your comment on #225
OL 5 version has changed to 5.3.1 in this PR

@Yakoust Yakoust merged commit 582c04d into quentinlampin:next Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: LGTM Looks good to me
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants