search

savoirfairelinux / ringme.js

A library to display a « Ring Me » button on a website.
https://frama.link/ringme
GNU General Public License v3.0
4 stars 14 forks source link

Fix Issue #18 #28

Closed samnegri closed 7 years ago

samnegri commented 7 years ago

Changes proposed in this pull request:

Waiting chrome timeout to call '_ringMeClickEventHandler' to avoid synchronization issue

Status

How to verify this change

Additional notes

The function isRingSchemeSupported runs asynchronous code to verify if the browser supports RingMe but does not wait for it to be completed. Therefore when the assertion if (this.ringUriSchemeSupported === URI_SCHEME_STATE.SUPPORTED) is checked, no callback was called yet to update the state of ringUriSchemeSupported.

I added a workaround using setTimeout to wait enough time before verifying the ringUriSchemeSupported state.

ssirois commented 7 years ago

Hi @samnegri and thank you for this pull request.

After testing, it is working as expected under Chromium and its derivatives. Tested under the following:

That being said, there is a regression with other browsers since they seem faster to react so the user is sent to the URL specified by the button before the event is fired.

When you hit the back button, event occurs and we then see the alert appear on screen.

Tested under the following:

samnegri commented 7 years ago

Hi @ssirois, I couldn't help to notice that you have an awesome name :smile:

Also, I'd like to say that this is my first contribution to an opensource project, so please, feel free to point anything that could help me to improve my contributions to the community.

About this issue, sorry about that regression, I should have considered testing in other browsers. I belive that everything is ok now, all I had to do was:

Best regards!

ssirois commented 7 years ago

@samnegri your doing great with your contribution. And as a side note, we should have provide a test suite from the start. It's 2017! I need to document myself on the techniques we could use to define containers or something in which we could execute tests on different browsers and Ring combinations.

I just sent you a quick PR to post-pone the integration of the keyword const for now and see how we can (in a new issue) discuss the matter of var, let, const (see https://github.com/samnegri/ringme.js/pull/1)

I see where you go with that and not using JS as my main language, I'd like to get your input on this, while keeping it separated in another issue.

Otherwise: everything works as expected. I'm waiting for your approval or refusal of my PR to merge this on the trunk!

Thank you very much for your work.

samnegri commented 7 years ago

Thanks for your review, I did not noticed that it was only using var

I believe that it should use the const keyword whenever possible to keep the variables immutable and avoid state change, as this makes it easier to understand and debug the code.

When this is not possible, I recommend using the let keyword instead ofvar, so the scope of this variable will be restricted to the block in which it was declared, that is, its state will not be propagated to other blocks, which could cause inconsistencies.

As for var, I believe it will rarely be necessary to use it, because when it is necessary to access a variable in a different scope, there are other ways to do this, such as by parameter, create an attribute or even apply the context to the function with .apply ()

If you agree with these points, I can help you migrate to this format, let me know if you open an issue

ventilooo commented 7 years ago

Hi @samnegri, I totally agree with your point. Could you please open an issue for that topic in order to clarify what refactor is needed.

Best regards :ok_hand: