Skip to content

Standard timers - fix for #4384 - #4389

Merged
vakrilov merged 1 commit into
NativeScript:masterfrom
WebReflection:master
Jun 15, 2017
Merged

Standard timers - fix for #4384#4389
vakrilov merged 1 commit into
NativeScript:masterfrom
WebReflection:master

Conversation

@WebReflection

@WebReflection WebReflection commented Jun 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Specifications define timers capable of accepting optional parameters.
https://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers

This PR goal is to standardize such behavior in NativeScript too.

To help the rest of the community review your change, please ensure:

@ns-bot

ns-bot commented Jun 15, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

@WebReflection WebReflection mentioned this pull request Jun 15, 2017
Closed
@ns-bot

ns-bot commented Jun 15, 2017

Copy link
Copy Markdown

Please sign CLA at http://www.nativescript.org/cla

@ns-bot ns-bot added the cla: no label Jun 15, 2017
@ns-bot

ns-bot commented Jun 15, 2017

Copy link
Copy Markdown

CLA signature found, happy contributing!

@ns-bot ns-bot added cla: yes and removed cla: no labels Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@NativeScript NativeScript deleted a comment from ns-bot Jun 15, 2017
@vakrilov

vakrilov commented Jun 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hi @WebReflection thanks for the contribution!

Would you mind adding a unit test or two about the optional params.
The rest of the timer unit tests are here - each test is defined in a separate exported function with the test prefix.

You can check the development workflow readme for tip on how to build and run the tests locally.

@WebReflection

WebReflection commented Jun 15, 2017

Copy link
Copy Markdown
Contributor Author

hi @vakrilov I have squashed tests within this PR, I hope that's fine.

The NativeScript build in Travis CI is like this on master anyway

build status

I am not expecting to see it green after this PR 😄

Mind having a double look to it?

Cheers.

@vakrilov

vakrilov commented Jun 15, 2017

Copy link
Copy Markdown
Contributor

run ci

vakrilov
vakrilov suggested changes Jun 15, 2017
View reviewed changes
Comment thread tests/app/timer/timer-tests.ts Outdated
// >> (hide)
counter += arg === rnd ? 1 : -1;
// << (hide)
}, 50);

@vakrilov vakrilov Jun 15, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pass the rnd param here

@vakrilov

vakrilov commented Jun 15, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hey @WebReflection.
I hope we can make Travis green again soon.
In the mean time - I ran the tests on our local Jenkins CI and one test failed(see the comment).
Apart from that the PR is looking great.

@WebReflection
Standard timers - fix for NativeScript#4384
55e657c 
Specifications define timers capable of accepting optional parameters.
https://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers

This PR goal is to standardize such behavior in NativeScript too.
@WebReflection

WebReflection commented Jun 15, 2017

Copy link
Copy Markdown
Contributor Author

added the rnd to the setInterval too, apologies for the silly failure, an extra Ctrl+s after an accidental Ctrl+z might have passed through.

Best Regards

@vakrilov

vakrilov commented Jun 15, 2017

Copy link
Copy Markdown
Contributor

run ci

@WebReflection

WebReflection commented Jun 15, 2017

Copy link
Copy Markdown
Contributor Author

@vakrilov ... you keep saying that ... I'm not sure I understand what you mean by run ci but if you're talking to bots, I'm OK with that, have a nice conversation 😆

@WebReflection

WebReflection commented Jun 15, 2017

Copy link
Copy Markdown
Contributor Author

screenshot from 2017-06-15 21-01-29

🍾 🎉

@vakrilov

vakrilov commented Jun 15, 2017

Copy link
Copy Markdown
Contributor

Yup - run ci triggers CI in Jenkins.
Merging - thanks for the great contribution!

@vakrilov
vakrilov merged commit 7e39bfb into NativeScript:master Jun 15, 2017
@jlooper

jlooper commented Jun 15, 2017

Copy link
Copy Markdown

@WebReflection Congratulations! This PR has earned you a Contributor badge, reserved for the best community PRs! DM me your t-shirt size and address on Slack and I'll be happy to send you some swag!
contrib

@lock

lock Bot commented Aug 27, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked and limited conversation to collaborators Aug 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
@vakrilov vakrilov vakrilov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@WebReflection @ns-bot @vakrilov @jlooper