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

UXUI-284 Countdown Component #52

Merged
merged 23 commits into from
Aug 11, 2015
Merged

UXUI-284 Countdown Component #52

merged 23 commits into from
Aug 11, 2015

Conversation

jodiedoubleday
Copy link
Contributor

What does this PR do? (please provide any background)

  • Creates a countdown component which allows a single date to be entered and counts down the seconds, minutes, hours and days between then and the current time.

What tests does this PR have?

  • All tests can be found in the tests folder of the component
  • To run the tests use npm test

How can be this tested?

  • To manually test, run grunt docs from a local copy.
  • The docs will start in your browser, scroll down to "countdown" and you will see a working version. You can edit the code on the docs and change the date to whatever you see fit.

Screenshots / Screencast

screen shot 2015-08-03 at 12 13 04

#### What gif best describes how you feel about this work?

Countdown


Developer Definition of Done/Quality Checklist (for PR author to complete BEFORE code review):

Software Engineer or Developer review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve run all the tests locally and they pass.

Software Engineer or project guru review:

  • I’ve witnessed the work behaving as expected (this could be on the authors machine or screencast).
  • I’ve checked for coding anti-patterns.
  • I’ve checked for appropriate test coverage.
  • I’ve run all the tests locally and they pass.

@HeinBehrenshx
Copy link
Contributor

Taking SE

@connormeredith
Copy link
Contributor

Taking Dev

var pad = function (number) {
number = Math.abs(Math.floor(number));
if (number > 9) {
return number;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be returning a string so that the output of this function is a consistent type. i.e return '' + number;

this.CountdownManager.start(this.callback);
this.timer.tick(CountdownManager.countdownInterval);
assert.equal(this.callback.called, true);
// expect(this.callback.called).to.be(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this?

@connormeredith
Copy link
Contributor

src/components/countdown/__tests__/countdownManager-test.js Should contain tests for every function.

@connormeredith
Copy link
Contributor

I'm happy with this once the above comments have been addressed. Also should the lozenge work be in this PR or should it be in a separate PR?

@HeinBehrenshx
Copy link
Contributor

+1

@connormeredith
Copy link
Contributor

Just one minor point, the references to Countdown in src/components/countdown/code/lib/countdown.js should be in lowercase as they are not being instantiated. I.e Countdown.untilString should be countdown.untilString. Other than that looking good 👍

@connormeredith
Copy link
Contributor

👍 Looks good dude

jodiedoubleday pushed a commit that referenced this pull request Aug 11, 2015
UXUI-284 Countdown Component
@jodiedoubleday jodiedoubleday merged commit 2a5031e into master Aug 11, 2015
@jodiedoubleday jodiedoubleday deleted the UXUI-284 branch August 11, 2015 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants