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

Eslint, leaflet lock, code cleaning, bugfixes #200

Merged
merged 13 commits into from
Sep 20, 2018

Conversation

DonNicoJs
Copy link
Member

Hi, first of all thanks for you hard work.

I know this is a big change, and I know this is a lot to read.

We use Vue2Leaflet in many of our project (ie: https://vuepeople.org/ ) and it's a great tool.
Since we use it a lot and we also pushed a bit of it's limit I dug deeper in the code base to fix some issues.

I noticed that no linter was added in the project so I dropped in a standard eslint config for vue projects and I discovered several linting / not linting problems, I am listening them not for blaming just to explain why I am bringing in such a big PR with so much rewritten.

  • Duplicated methods (block) declarations
  • Duplicated functions (one of which make the plugin crash on removing a geo-json layer at runtime )
  • == instead of ===
  • v-for without keys or with wrong keys
  • leaflet version is not locked and 1.3.2 (released 2 days ago ) break the plugin - not exporting the L object on import -
  • no reliable way to know when leaflet is ready ( tentative solution added )

There is also still to address the following problems:

  • v-tooltip generate and paint one DOM element each time is instantiated, with more than 100 tooltip with a complex markup the page overloads even on big machine

  • findRealParent is a great function but as it is it does not allow to have some combination of nesting ( ie feature group inside a marker -component- ) which could be a legit combination .

And we plan to address them since we are actively maintaining projects based on this code.

I am opening this PR in the hope that we can collaborate actively on this project. it's such a good plugin with a lot of uses / third party plugins.

Please let me know how you want to move forward, if you have time to check the PR ( in a reasonable timeframe ) and your thoughts about it.

Once again your work is awesome, and enabled us to build amazing UI, and we/I am here to help you improve it.

@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Jul 23, 2018

Hi @lordfuoco,

This PR looks awesome and implement plenty of things I would love to have time to do.

I'm sure you put a lot of effort and work into it and will try to review it and merge it before the end of the month.

The two issues you raised are issues/limitations I'm aware of but as well, didn't have time to investigate/solve properly. I think the best way forward is to open an issue for each of these problems with a clear description or test for it to make the collaboration easier.

Again thanks for the great work!

Mickaël

@DonNicoJs
Copy link
Member Author

Hi @KoRiGaN

Thank you! please let me know if there is anything that you would like altered in the PR.

I will open those issue and try to address them separately, starting from the v-tooltip one since I have it addressed with a workaround on the vue-people codebase

Nico.

@patrickcate
Copy link
Contributor

Any update on merging this PR?

@jperelli
Copy link
Contributor

jperelli commented Aug 22, 2018

Hi @KoRiGaN do you need help on testing this one for approval?

@DonNicoJs
Copy link
Member Author

Hi @KoRiGaN I am writing to let you know that since this PR is not going in we are actively thinking to release on npm our fork, where we could keep improving this good codebase.
This is not how we wanted to proceed because fragmenting the resources is not the best but we work daily on this code and we need to move forward.

Please let me know if you have any other suggestions.

@nparley
Copy link
Contributor

nparley commented Sep 10, 2018

@KoRiGaN has done a great job with this but I wonder if there is not a more active fork then it will actually cause more code fragmentation. We are also considering using this in my team and would probably go the way of @lordfuoco and create our own fork so we could maintain the code. The other option other than a fork is if @KoRiGaN adds a few member such as @lordfuoco to help manage the project.

@DonNicoJs
Copy link
Member Author

@nparley If we go the fork -> npm direction please do not create another fork but let's work together, the idea is that we would be active maintainers in the new one and accept PRs, answer issues (as i am doing here already )

@patrickcate
Copy link
Contributor

The other option other than a fork is if @KoRiGaN adds a few member such as @lordfuoco to help manage the project.

This seems like the best route to me if @KoRiGaN agrees. More developers collaborating together will make this library even stronger.

@DonNicoJs
Copy link
Member Author

@KoRiGaN Thank you for adding me to the team! In the end did you go through this PR ? Can we merge it?

@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Sep 20, 2018

@lordfuoco I went through it and am happy with the changes so merging it

@KoRiGaN KoRiGaN merged commit c6ad563 into vue-leaflet:master Sep 20, 2018
@KoRiGaN
Copy link
Collaborator

KoRiGaN commented Sep 20, 2018

Will probably push a new release soon. It would be great If you could create new release in the future

@DonNicoJs
Copy link
Member Author

DonNicoJs commented Sep 20, 2018

@KoRiGaN yes I can take care of the releases too, there is channel that you prefer where can communicate ?

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.

5 participants