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

added MapIdle and BoundChanged Listeners #107

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Conversation

thiasos
Copy link

@thiasos thiasos commented Oct 2, 2023

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

@thiasos thiasos changed the title getBound in "Synchromized Mode" without wait Event from Client added MapIdle Listener and getBound in Synchromized Mode without wait Event from Client Oct 2, 2023
@thiasos
Copy link
Author

thiasos commented Oct 2, 2023

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
Copy link
Member

paodb commented Oct 2, 2023

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 Event filtering and debounce everywhere vaadin/flow#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
Copy link
Author

thiasos commented Oct 3, 2023

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 thiasos force-pushed the master branch 2 times, most recently from 7e399b4 to a27eb61 Compare October 3, 2023 09:51
@thiasos
Copy link
Author

thiasos commented Oct 3, 2023

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

@thiasos thiasos changed the title added MapIdle Listener and getBound in Synchromized Mode without wait Event from Client added MapIdle and BoundChanged Listeners Oct 3, 2023
@javier-godoy
Copy link
Member

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.

@@ -76,6 +77,11 @@ public GoogleMap(String apiKey, String clientId, String language) {
if (!StringUtils.isEmpty(language)) {
this.getElement().setAttribute("language", language);
}
getElement().addEventListener("google-map-ready", event -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this part (lines 80 to 84)? the listeners and the firing of the 'google-map-bounds_changed' should be done in the web-component part of the add-on. I will do that update and once that's ready, we can merge the rest of your changes.

@sonarcloud
Copy link

sonarcloud bot commented Oct 4, 2023

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 paodb merged commit fa3d4d2 into FlowingCode:master Oct 4, 2023
4 of 5 checks passed
@paodb
Copy link
Member

paodb commented Oct 12, 2023

We just released new version 1.10.0 including these changes.

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