Added Icon component to easily add dynamic composed marker icons #268

byWulf · 2018-11-04T00:14:04Z

As suggested in #267 I added a LIcon component which can be placed inside a marker for dynamically generating the icon.

This way you can bind dynamic values to properties of the icon (f.e. icon size to make the icon smaller on zoom out) and you can place HTML code inside the <l-icon> tag to transform it into a L.divIcon, which updates its content as Vue rerenders the slot template.

I also added an example file (examples/src/components/Icon.vue) to see the different implementation possibilities.

byWulf · 2018-11-04T00:16:24Z

src/components/LIcon.vue

+      for (let i = 0; i < keys.length; i++) {
+        this.$watch(keys[i], (newVal, oldVal) => {
+          //TODO: why is watch triggered on foreign props when changing icon size in my example?
+          if (JSON.stringify(newVal) === JSON.stringify(oldVal)) {


I'm relatively new to Vue, so if someone could help me out how to avoid this line, I would be very thankful!

Problem is, when sliding the slider in the example, the watcher is also triggered for the fourth icon elements properties, but it should only be called for the iconSize and iconAnchor properties of the third icon.

Vue pass any prop written in the component template to the $props, even if not defined in the props definition.
Also you don't need this method, for an inspiration please take a look at how the function in utils/propsBinder.js works and how is used in any other component.

The gist of it is that you can write a function in the methods called: setPROPNAME and that function will be called only when your props change.

Thanks for your reply and your awesome code review!

I updated the code (marked all props as custom so the component functions will get called, because Icon doesn't have setters on an existing Icon), but when you check it out and test the example, you will notice that it still doesn't behave correct.

When changing the slider, the first icon will get recreated twice (because two props are affected by the slider), AND the second icon is also recreated (because of the iconAnchor prop). This prop shouldn't change.
Also when you change the text, again the first icon is recreated twice (because of the iconSize and iconAnchor props) and the second one also twice (first correctly because of the html change but then also because of the iconAnchor prop).

------ Page load: 1541353711324 - recreate icon 1 (initial) 1541353711327 - recreate icon 2 (initial) ------ Changing slider: 1541353713991 - recreate icon 1 (iconSize) 1541353713992 - recreate icon 1 (iconAnchor) 1541353713994 - recreate icon 2 (iconAnchor) ------ Changing text: 1541353720983 - recreate icon 1 (iconSize) 1541353720985 - recreate icon 1 (iconAnchor) 1541353720986 - recreate icon 2 (iconAnchor) 1541353720988 - recreate icon 2 (html slot)

So two problems:

When two or more props on the same component are change simultanuosly, is it possible to trigger a recreation only once?

Why are all props detected as changed (even on different icon components), when only one of them changes?

I hope you can help me out here too! :)

@byWulf this could be an issue with the fact that the icon component gets recreated by his parent ( marker) you can check this by verifying that the mounted / updated lifecycle hooks get called.

there is a lot of performance issues with icon and markers but I have the feeling that you can get it right ( and I will help you to do so )

Really appreciate all your efforts and attitude to this!
If you want to discuss this more faster ping me on Discord, in VueLand you can find me by the name of DonNico

@lordfuoco I spent the last night on analysing and tweaking the performance.

Problem 1 was in the example, that I didn't bind computed values to the component, but directly binded arrays (<l-icon :icon-anchor="[0, 50]" />). This led to vue thinking the value has changed on every check. <l-icon :icon-anchor="iconAnchor" /> + `computed: { iconAnchor() { return [0, 50]; }}`` solved this.

Problem 2 with the "if multiple props were affected by a single change, the icon gets recreated also multiple times" was solved by scheduling a recreation on $nextTick and remember in a data variable, if recreation in this tick already happend, if multiple recreations were scheduled.

Also I added another tweak: If only the html content changed, I just swap the HTML in the DOM and don't recreate the icon itself.

In my real world project (880 markers, where only few of them have individual divIcons) it was recommended to put the default icons in data variables and bind them to the icon prop of the marker and add an v-if to the l-icon component. Works like a charme now :)

Looking forward for your feedback/code review on the current status, so we could release it soon :)

@byWulf Amazing work! I will need to go through it thoroughly, but I think that by the end of the day I should be able to check it

DonNicoJs

Very good work! Please address my comments and then we can proceed to merge this.
We will need to do some performance tests but it seems okay to me.
Please remember to run npm run lint or enable an automatic linter in your editor of choice :)

DonNicoJs · 2018-11-04T11:05:31Z

src/components/LIcon.vue

+  name: 'LIcon',
+  mixins: [Options],
+  props: {
+    iconUrl: String,


On the props please define the default value even if null or undefined this triggers linting errors :)

DonNicoJs · 2018-11-04T11:09:44Z

src/components/LIcon.vue

+      for (let i = 0; i < keys.length; i++) {
+        this.$watch(keys[i], (newVal, oldVal) => {
+          //TODO: why is watch triggered on foreign props when changing icon size in my example?
+          if (JSON.stringify(newVal) === JSON.stringify(oldVal)) {


Vue pass any prop written in the component template to the $props, even if not defined in the props definition.
Also you don't need this method, for an inspiration please take a look at how the function in utils/propsBinder.js works and how is used in any other component.

The gist of it is that you can write a function in the methods called: setPROPNAME and that function will be called only when your props change.

DonNicoJs · 2018-11-06T09:51:07Z

@byWulf Merged, I did not find anything to complain about in the code ;)

byWulf · 2018-11-06T10:49:45Z

@lordfuoco Thanks again for the great review and response!

laurencei · 2018-12-01T09:05:34Z

@byWulf @lordfuoco - thanks for this.

This is the exact feature I was looking for today - so glad it's been implemented. Seems to be working perfectly for me so far.

DonNicoJs · 2018-12-01T09:57:19Z

@byWulf I am almost done writing the docs, the base of it. When I am done would you like to refine the section about l-icon ? with tips and stuff, since you wrote it :)

byWulf · 2018-12-01T15:24:43Z

@lordfuoco just let me know when I can help :)

DonNicoJs · 2018-12-02T09:11:28Z

@byWulf the documentation is online :) the docs are run with docsify

blackspike · 2019-01-29T14:20:32Z

Hi all, I'm not sure the custom text is working in the div icons, it only seems to show an image – not the divIcon content.

The example isn't showing text for me either

DonNicoJs · 2019-01-30T07:44:31Z

@blackspike the example that you linked does not use the custom icon, maybe you linked not the one that you mentioned ? Can we see a bit of your component code ?

blackspike · 2019-01-30T09:25:33Z

Apologies the link does not go directly to the example, same page but last example in the list Custom Marker Icons , there's a text box called 'Custom text' that doesn't seem to work.

MagicTrevor · 2019-02-22T01:30:01Z

On the example it seems to be throwing the same error I am getting in my project which seems to prevent it from working correctly:

I am under the impression that the following should work:

<l-marker :key="grid.id" :lat-lng="grid.getCenter()">
  <l-icon class-name="zone-label">
    <div class="zone-text">test</div>
  </l-icon>
</l-marker>

DonNicoJs · 2019-02-22T12:43:32Z

I will give a look here @blackspike @MagicTrevor.

Sorry for the delay I am a bit swamped at the moment. if anyone wants to take this please let me know it would be really helpful

byWulf · 2019-02-22T13:16:42Z

Hey @DonNicoJs , just started looking into this when you wrote that (telapathy :D). And I found the bug and created a PR which fixes divIcons :)

See #342

@blackspike @MagicTrevor

MagicTrevor · 2019-02-22T15:52:50Z

Thanks @byWulf! If this is resolved will there be a patch release? Or an updated 2.0.0-beta?

DonNicoJs · 2019-02-24T13:10:58Z

@MagicTrevor I think I am releasing 2.0.0 as not beta soon with this fix included. In this week I think. I just need to update the docs

byWulf added 3 commits November 4, 2018 00:50

Added LIcon component for dynamically create divIcons

590be2e

Cleanup

005de8a

Cleanup

94a9588

byWulf commented Nov 4, 2018

View reviewed changes

DonNicoJs reviewed Nov 4, 2018

View reviewed changes

byWulf and others added 5 commits November 4, 2018 18:41

Fixed Lint errors. Reworked change detection. WIP!

0174dc6

More debugging

c10d2a4

Fixed typehinting

27c8aae

Fixed performance issue, when the icon was recreated too often.

03887ec

Removed unused import from example

0ba0035

DonNicoJs approved these changes Nov 6, 2018

View reviewed changes

DonNicoJs merged commit ac4f076 into vue-leaflet:master Nov 6, 2018

byWulf mentioned this pull request Feb 22, 2019

Fixes divIcons #342

Merged

Added Icon component to easily add dynamic composed marker icons #268

Added Icon component to easily add dynamic composed marker icons #268

Uh oh!

Conversation

byWulf commented Nov 4, 2018

Uh oh!

byWulf Nov 4, 2018

Choose a reason for hiding this comment

Uh oh!

DonNicoJs Nov 4, 2018

Choose a reason for hiding this comment

Uh oh!

byWulf Nov 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DonNicoJs Nov 5, 2018

Choose a reason for hiding this comment

Uh oh!

byWulf Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

DonNicoJs Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

DonNicoJs left a comment

Choose a reason for hiding this comment

Uh oh!

DonNicoJs Nov 4, 2018

Choose a reason for hiding this comment

Uh oh!

DonNicoJs Nov 4, 2018

Choose a reason for hiding this comment

Uh oh!

DonNicoJs commented Nov 6, 2018

Uh oh!

byWulf commented Nov 6, 2018

Uh oh!

laurencei commented Dec 1, 2018

Uh oh!

DonNicoJs commented Dec 1, 2018

Uh oh!

byWulf commented Dec 1, 2018

Uh oh!

DonNicoJs commented Dec 2, 2018

Uh oh!

blackspike commented Jan 29, 2019

Uh oh!

DonNicoJs commented Jan 30, 2019

Uh oh!

blackspike commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MagicTrevor commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DonNicoJs commented Feb 22, 2019

Uh oh!

byWulf commented Feb 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MagicTrevor commented Feb 22, 2019

Uh oh!

DonNicoJs commented Feb 24, 2019

Uh oh!

Uh oh!

byWulf Nov 4, 2018 •

edited

Loading

blackspike commented Jan 30, 2019 •

edited

Loading

MagicTrevor commented Feb 22, 2019 •

edited

Loading

byWulf commented Feb 22, 2019 •

edited

Loading