Skip to content

Fix bug in LIcon not properly referencing parent container's map object #657

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

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

fernando-almeida
Copy link
Contributor

The current code as is only works if the Licon is directly beneath another Vue2Leafleft component.
This fix addresses using the correct real parent already determined but not used when binding props upon mounting of the component.

…ct, in particular when using a custom vue-component.
@DonNicoJs
Copy link
Member

@mikeu could you review this one? Since you are the l-icon expert?

@mikeu
Copy link
Contributor

mikeu commented Feb 7, 2021

@fernando-almeida thank you for this submission, it looks good to me!

@DonNicoJs I do have two questions for you before we merge this:

First, it throws an explicit error when the parent container is not found. This seems like a good practice in general, but it is different behaviour from all of the other components. Do you think we should remove that exception here for consistency, or update the other components to have this improvement?

Second—and this is not actually about the PR, just raised by looking through it—the LIcon component has for a long time now made its parentContainer property a reactive part of its Vue data, which none of the other components do I don't think. Should we also remove that while updating the LIcon here? There isn't any reason that the template should need to be able to reference that property, is there? I suppose technically that would be a breaking change if anybody has written code that relies on it for some reason, though...

@DonNicoJs
Copy link
Member

@fernando-almeida thank you for this submission, it looks good to me!

@DonNicoJs I do have two questions for you before we merge this:

First, it throws an explicit error when the parent container is not found. This seems like a good practice in general, but it is different behaviour from all of the other components. Do you think we should remove that exception here for consistency, or update the other components to have this improvement?

Second—and this is not actually about the PR, just raised by looking through it—the LIcon component has for a long time now made its parentContainer property a reactive part of its Vue data, which none of the other components do I don't think. Should we also remove that while updating the LIcon here? There isn't any reason that the template should need to be able to reference that property, is there? I suppose technically that would be a breaking change if anybody has written code that relies on it for some reason, though...

I believe we should have avoided to put it in data 😅 my mistake in not noticing it before, that being said it's better to leave it as it is right now. We should take care to not do this mistake again in the vue3 version and if we did correct it ASAP.

About the error it's true that we do not have in the codebase but I think it would offer a good developer experience

@DonNicoJs
Copy link
Member

@mikeu I'll merge this, and we can take care of any followup work in a further PR!

@DonNicoJs DonNicoJs merged commit 8dd879e into vue-leaflet:master Feb 26, 2021
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.

3 participants