FlowingCode / GoogleMapsAddon

Vaadin Addon based on Google Maps Web Component
https://www.flowingcode.com/en/open-source/
Apache License 2.0
19 stars 7 forks source link

added MapIdle and BoundChanged Listeners #107

Closed thiasos closed 9 months ago

thiasos commented 9 months ago

added MapIdle Listener and getBound in Synchromized Mode without wait Event from Client

thiasos commented 9 months ago

I see that commit message is invalid. I've checked the Convention and i think that message is correct. Let me say how can I fix the commit message and I resubmit. Thanks

Bruno

paodb commented 9 months ago

Hi @thiasos , thanks for taking the time for this contribution. We tested the changes but it was failing when trying to get the "bounds" property value, as the property was null. Below, some observations regarding the PR:

  1. The Synchronize annotation in getBoundsSync method, needs the property="bounds" because otherwise it infers the property name from the getter.
  2. However, by synchronizing the property it would fire many times, so it has to be debounced (but @Synchronize does not support debounce https://github.com/vaadin/flow/issues/4016)
  3. One approach will be to implement a custom event that communicates the bounds values
  4. Regarding the commit message:

    I see that commit message is invalid. I've checked the Convention and i think that message is correct. Let me say how can I fix the commit message and I resubmit. Thanks

The second commit "feat: add google-map-idle listener" is valid, however the checker would complain because of the other commit. Squashing the commits will be the way to go here.

thiasos commented 9 months ago

Hi @paodb , thanks for you suggestion. I'm using this method in previous version of library (1.8.2) with name "getBound" and works correctly. During merge with master i've noticed of duplicated method and I've renamed it but, my fault, I did'nt exexute test again. I'll follow you suggestion and try to create a new "BoundsChanged" event. Which is project policy in this case? Is better to abort this PullRequest or leave it open and modify once all modification done?

Thanks for your time

thiasos commented 9 months ago

I've reviewed the code. Should work correctly now.

javier-godoy commented 9 months ago

Which is project policy in this case? Is better to abort this PullRequest or leave it open and modify once all modification done?

Hello. It's fine if you force-push the PR with the modified code.

Regarding our commit practices, we have adopted the principle of "logically atomic commits", i.e. commits that stand by themselves, are big enough to add value to the project, and small enough to read, review and revert.

BTW, you can disregard the message stating that "Version 1.9.1-SNAPSHOT cannot contain new features." It's simply a reminder that we should update the version to 1.10.0-SNAPSHOT before merging the new feature.

sonarcloud[bot] commented 9 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

paodb commented 9 months ago

We just released new version 1.10.0 including these changes.