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

gmap-vue.js needs rebuild to include new components #44

Closed
davydnorris opened this issue Aug 2, 2020 · 20 comments · Fixed by #56 or #57
Closed

gmap-vue.js needs rebuild to include new components #44

davydnorris opened this issue Aug 2, 2020 · 20 comments · Fixed by #56 or #57
Assignees
Labels
bug Something isn't working pr-merged solved The problem presented is considered solved and the issue closed
Projects

Comments

@davydnorris
Copy link

@diegoazh - just tried to test the examples and they aren't working because the new components aren't added to gmap-vue.js

Also the drawing manager examples aren't even loading the Google map

I also something isn't really wrong with the edits that were made to drawing-manager2.html - there is a callback slot in the drawing manager component that works the same way as the auto-complete and it's gone missing in the example, and the functions provided by the slot are being called without the slot prefix, so they do not exist

@close-issue-app
Copy link

This issue is closed because it does not meet our issue template. Please read it.

@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@diegoazh - just tried to test the examples and they aren't working because the new components aren't added to gmap-vue.js

Also the drawing manager examples aren't even loading the Google map

I also something isn't really wrong with the edits that were made to drawing-manager2.html - there is a callback slot in the drawing manager component that works the same way as the auto-complete and it's gone missing in the example, and the functions provided by the slot are being called without the slot prefix, so they do not exist

@davydnorris I land your features, and the code works very well, please use the documentation examples to test it, if you test with the HTML examples (they will be removed soon) you need to build your own gmap-vue.js. I don't commit this file because it is a built file. If you want to test the new features without compiling your own file please use the following option https://cdn.jsdelivr.net/npm/gmap-vue@1.2.1/dist/gmap-vue.min.js
I test it recently and all the features that you added are working very well 👍🏽.
Sorry for the bot.

@davydnorris
Copy link
Author

I have tested myself and the drawing manager html examples aren't loading the maps. I have adjusted the first one and am now trying to get the custom toolbar example to work.

@davydnorris
Copy link
Author

BTW I fixed the heatmap autoload - you can remove the button and change the script section to:

    <script>
      Vue.use(GmapVue, {
        load: {
          key: 'AIzaSyDf43lPdwlF98RCBsJOFNKOkoEjkwxb5Sc',
          libraries: 'visualization'
        },
      });
  
      document.addEventListener('DOMContentLoaded', function () {
        new Vue({
          el: '#root',
          data: {
            center: {lat:4.5, lng: 99}, 
          },
          computed: {
            google: (GmapVue ? GmapVue.gmapApi : null),
            markers() {
              if (this.google) {
                return [
                  { location: new google.maps.LatLng({ lat: 3, lng: 101 }), weight: 100 },
                  { location: new google.maps.LatLng({ lat: 5, lng: 99 }), weight: 50  },
                  { location: new google.maps.LatLng({ lat: 6, lng: 97 }), weight: 80 }
                ];
              }
              return [];
            }
          }
        });
      });
  
    </script>

@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@davydnorris If you continue with this problem please can you provide more details to understand how you try to test these functionalities?
Please check the following captures that I recorded a few minutes ago:
https://www.loom.com/share/ac4af1732d9b48cba05b3ea935ca79bc
https://www.loom.com/share/7a01249dce814bbd8cc4bd8822ea7868
https://www.loom.com/share/f4048e06fcea4b25b8507109fa2a9bb4

@davydnorris
Copy link
Author

Video 1 is technically correct but you can get the Heatmap points to autoload if you use the script above.
Video 2 is correct behavior
Video 3 should show a custom toolbar instead of the default and this is not working

@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@davydnorris ok, if that is the expected behaviour of the point 3, I never see it working. When I tried the HTML examples that's what I saw. Can you re-check that?

@diegoazh diegoazh reopened this Aug 3, 2020
@davydnorris
Copy link
Author

yeah it's not working - I'm trying to fix it now

@diegoazh diegoazh added bug Something isn't working under investigation and removed closed by bot labels Aug 3, 2020
@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@davydnorris I remove this line because return false and nothing was displayed on the map 🤷🏻‍♂️. I set it to true in the debugger and the toolbar options appears in the map.

// drawing-manager.js
// line 68
    options.drawingControl = !this.$scopedSlots.default;

@davydnorris
Copy link
Author

davydnorris commented Aug 3, 2020

So the line above checks to see whether you have provided your own custom toolbar or not by checking if there is something in the slot.

The drawing manager vue file defines two functions on the slot, and you need to add v-slot="" to the outer template wrapper or component in order to access them, or explicitly get them with v-slot="{ deleteSelection, setDrawingMode }"

When you do this, Vue thinks the slot is now scoped, so that's why it's checking the scoped slot variable.

@davydnorris
Copy link
Author

davydnorris commented Aug 3, 2020

@diegoazh I found the issue - the pages are using an old release of Vuejs and slot naming is now different. This meant that the drawing manager was always dropping into the default slot, which has a class that makes it invisible. Updating to Vuejs 2.6.x fixed the slot naming and it started working.

I have now fixed all examples and they work well. Please see attached
examples-fixed.zip

I also made the drawing toolbar a little bit nicer

@diegoazh diegoazh assigned diegoazh and davydnorris and unassigned diegoazh Aug 3, 2020
@diegoazh diegoazh added this to In progress in gmap-vue Aug 3, 2020
@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@davydnorris excellent, today a will check the new examples and update Vue on the project, thanks for your efforts 👏🏽

@davydnorris
Copy link
Author

I'll also do some content for the documentation around this as well.

@davydnorris
Copy link
Author

One thing that really trapped me, and we should definitely document, is the fact that the map component has two slots - the default slot is wrapped in a class that sets display: none; so by default any component you add to your map will be invisible.

This is OK for most of the supplied components that interact directly with the Google map object, but it's not good if you want to bring up things like toolboxes etc (as I found out!!).

There is a second slot named "visible" that must be used if you want to display content within the responsive wrapper for the map, hence that's why you'll see this in the second drawing manager example. It's actually not required in the first example because the default toolbox is part of the Google map object

@diegoazh
Copy link
Owner

diegoazh commented Aug 3, 2020

@davydnorris we need to change something in the drawing-manager.js? 🤔

@davydnorris
Copy link
Author

No - it should all work fine with current as far as I know. I had to kill my git clone and pull fresh from here because the pull was messed up, so I have the current master

diegoazh added a commit that referenced this issue Aug 5, 2020
We change examples and fix the drawing-manager.js adding the previously
removed line 68.

close: #44
@diegoazh diegoazh linked a pull request Aug 5, 2020 that will close this issue
gmap-vue automation moved this from In progress to Done Aug 5, 2020
diegoazh added a commit that referenced this issue Aug 5, 2020
* fix(documentation): change heatmap and drawing map examples

We change examples and fix the drawing-manager.js adding the previously
removed line 68.

close: #44

* docs(documentation): add documentation for lazy loading and slots

close: #44

* docs(documentation): upgrade documentation version and gmap-vue version on examples

* docs(gmap-vue): upgrade gmap-vue version

* test(gmap-vue): fix tests to the new api name

* docs(documentation): add documentation explain how to access google maps api

* chore(all): update package-lock

* chore(root): add script test to travis yaml file
@helpr helpr bot added the pr-available label Aug 5, 2020
@diegoazh diegoazh linked a pull request Aug 5, 2020 that will close this issue
diegoazh added a commit that referenced this issue Aug 5, 2020
)

* fix(documentation): change heatmap and drawing map examples

We change examples and fix the drawing-manager.js adding the previously
removed line 68.

close: #44

* docs(documentation): add documentation for lazy loading and slots

close: #44

* docs(documentation): upgrade documentation version and gmap-vue version on examples

* docs(gmap-vue): upgrade gmap-vue version

* test(gmap-vue): fix tests to the new api name

* docs(documentation): add documentation explain how to access google maps api

* chore(all): update package-lock

* chore(root): add script test to travis yaml file
@helpr helpr bot added pr-merged and removed pr-available labels Aug 5, 2020
@diegoazh
Copy link
Owner

diegoazh commented Aug 5, 2020

@davydnorris thank you so much for your efforts. I merge the changes and all is working fine, I also add more documentation and an acknowledgement for your contribution to the docs.
bitmoji

diegoazh added a commit that referenced this issue Aug 5, 2020
* fix(gmap-vue): fix hit and drawer map components and examples (#56) (#57)

* fix(documentation): change heatmap and drawing map examples

We change examples and fix the drawing-manager.js adding the previously
removed line 68.

close: #44

* docs(documentation): add documentation for lazy loading and slots

close: #44

* docs(documentation): upgrade documentation version and gmap-vue version on examples

* docs(gmap-vue): upgrade gmap-vue version

* test(gmap-vue): fix tests to the new api name

* docs(documentation): add documentation explain how to access google maps api

* chore(all): update package-lock

* chore(root): add script test to travis yaml file

* chore(root): add version badge to the readme (#58)
@davydnorris
Copy link
Author

davydnorris commented Aug 5, 2020

@diegoazh - thanks for merging this all in. I noticed you mentioned the toolbar isn't working - it works perfectly for me in my html example but I can't test the documentation version as I can't seem to find a working google map API key for the documentation site. Happy to help diagnose any problems with the examples in the documentation - it may also be useful to mention that in a regular application you would simply create a separate drawingToolbar.vue file and component - I had to do it like that in the example to make it one HTML file.

Also I like your use of gmapPromiseLazy in the heatmap - that's really good.

It may also be useful to show the computed version of the heatmap points because it's likely to be needed. There is one thing with heatmaps that catch you out - they are one of the few components where you must use the Google LatLng object. You can't use a generic object like { lat: 0, lng: 0 }.

Took me a while to figure that out because Markers will accept generic objects!

In practical examples you are most likely going to want to make the heatmap array reactive in some way and so it's not so simple as creating the array in the mounted() function - Vue will only pick up additions to arrays and make them reactive if you use push or a computed property, and typically your markers will be a static array of generic objects, but your heatmap weights will be dynamic and will need to be reactive, and will need to be Google LatLng objects.

@davydnorris
Copy link
Author

Oh and one last tiny thing - the discussion about the visible slot. This is not generic to GmapVue, it's specific to GmapMap so you may want to change the one word in that discussion so people know it's just the map component

@diegoazh
Copy link
Owner

diegoazh commented Aug 6, 2020

@diegoazh - thanks for merging this all in. I noticed you mentioned the toolbar isn't working - it works perfectly for me in my html example but I can't test the documentation version as I can't seem to find a working google map API key for the documentation site. Happy to help diagnose any problems with the examples in the documentation - it may also be useful to mention that in a regular application you would simply create a separate drawingToolbar.vue file and component - I had to do it like that in the example to make it one HTML file.

Also I like your use of gmapPromiseLazy in the heatmap - that's really good.

It may also be useful to show the computed version of the heatmap points because it's likely to be needed. There is one thing with heatmaps that catch you out - they are one of the few components where you must use the Google LatLng object. You can't use a generic object like { lat: 0, lng: 0 }.

Took me a while to figure that out because Markers will accept generic objects!

In practical examples you are most likely going to want to make the heatmap array reactive in some way and so it's not so simple as creating the array in the mounted() function - Vue will only pick up additions to arrays and make them reactive if you use push or a computed property, and typically your markers will be a static array of generic objects, but your heatmap weights will be dynamic and will need to be reactive, and will need to be Google LatLng objects.

@davydnorris don't worry,

  • The toolbar is working very well in the examples.
  • The warning about the toolbar component is because someone can be using the runtime-only and trying to reproduce the example and maybe it doesn't work.
  • I will add the points that you mention into the documentation

Thank you again.

Screen Shot 2020-08-06 at 09 05 22

@diegoazh diegoazh reopened this Nov 18, 2021
gmap-vue automation moved this from Done to In progress Nov 18, 2021
gmap-vue automation moved this from In progress to Done Nov 26, 2021
@diegoazh diegoazh added the solved The problem presented is considered solved and the issue closed label Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pr-merged solved The problem presented is considered solved and the issue closed
Projects
No open projects
gmap-vue
  
Done
2 participants