Feature/km/add recorder timer #116

resource11 · 2017-02-03T22:53:52Z

Code-review checklist

Documentation
- Interfaces describe how other programmers would use them.
- Code that is not obvious has documentation describing why
  that code is the way it is.

houndci-bot · 2017-02-03T22:54:02Z

client/styles/components/_components.answers.scss

+    width: 100%;
+    resize: none;
+    padding: 0.5rem 1rem;
+    @include regular;


Expected item on line 246 to appear before line 240. Rule sets should be ordered as follows: @extends, @includes without @content, properties, @includes with @content, nested rule sets

houndci-bot · 2017-02-03T22:54:03Z

client/styles/components/_components.answers.scss

+    border: 0.1rem solid $light-gray;
+    border-radius: $radius;
+    // min-height: 10rem;
+    width: 100%;


Properties should be ordered border, border-radius, color, font-size, padding, resize, width

houndci-bot · 2017-02-03T22:54:04Z

client/styles/components/_components.answers.scss

-      font-size: 1.4rem;
-      color: $text-color;
-    }
+  textarea{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:04Z

client/styles/components/_components.answers.scss


-  .c-text-answer{
-    padding: 2rem 3rem 1rem;
+.c-text-answer{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:05Z

client/styles/components/_components.answers.scss

+  50%{
+    opacity: 0;
+  }
+  100%{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:06Z

client/styles/components/_components.answers.scss

+  }
+  50%{
+    opacity: 0;
+  }


Rule declaration should be followed by an empty line

houndci-bot · 2017-02-03T22:54:06Z

client/styles/components/_components.answers.scss

+  0%{
+    opacity: 1;
+  }
+  50%{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:07Z

client/styles/components/_components.answers.scss

+@keyframes blink {
+  0%{
+    opacity: 1;
+  }


Rule declaration should be followed by an empty line

houndci-bot · 2017-02-03T22:54:07Z

client/styles/components/_components.answers.scss

-      opacity: 1;
-    }
+@keyframes blink {
+  0%{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:07Z

client/styles/components/_components.answers.scss

    }
  }
+  &.c-btn--stop:active{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:08Z

client/styles/components/_components.answers.scss

+    background: $incorrect;
+    span:before{
+      animation: blink 2s linear infinite;
+      -webkit-animation: blink 2s linear infinite;


Avoid vendor prefixes.

houndci-bot · 2017-02-03T22:54:08Z

client/styles/components/_components.answers.scss

-      border-color: darken($incorrect, 15%);
+    background: $incorrect;
+    span:before{
+      animation: blink 2s linear infinite;


Properties should be ordered -webkit-animation, animation

houndci-bot · 2017-02-03T22:54:08Z

client/styles/components/_components.answers.scss

-      background: darken($incorrect, 15%);
-      border-color: darken($incorrect, 15%);
+    background: $incorrect;
+    span:before{


Rule declaration should be preceded by an empty line
Begin pseudo elements with double colons: ::
Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:09Z

client/styles/components/_components.answers.scss

-    &.c-btn--stop:active{
-      background: darken($incorrect, 15%);
-      border-color: darken($incorrect, 15%);
+    background: $incorrect;


Expected item on line 212 to appear before line 206. Rule sets should be ordered as follows: @extends, @includes without @content, properties, @includes with @content, nested rule sets

houndci-bot · 2017-02-03T22:54:10Z

client/styles/components/_components.answers.scss


+  &.c-btn--stop{


Opening curly brace { should be preceded by one space

houndci-bot · 2017-02-03T22:54:10Z

client/styles/components/_components.answers.scss

    @include transition(all 0.1s ease);

-    &:hover{
+    &:before{
+      content: '';


Properties should be ordered background, border-radius, content, height, left, position, top, width
Prefer double-quoted strings

coveralls · 2017-02-03T22:58:06Z

Coverage increased (+0.005%) to 99.08% when pulling 1045615 on CLIxIndia-Dev:feature/km/add-recorder-timer into 4b645ce on atomicjolt:master.

BenHSmith13 · 2017-02-03T23:44:14Z

client/js/_player/components/common/recorder_timer.jsx

+    }
+  }
+
+  handleTimerCount = () => {


Why not just declare these as functions on the class instead of declaring them as lambdas? Like above with tick()

Is the way I've coded this not a best practice? I certainly welcome tips on JS/React best practices.

Ah, you mean on handleTimerCount. Will do!

BenHSmith13 · 2017-02-03T23:56:53Z

I moved the player style folders into a subfolder, and that is up on master. It is ignored by hound, but you may have some conflict with the moved files. Sorry

houndci-bot · 2017-02-07T18:19:13Z

client/tools/build/index.js

@@ -159,7 +159,8 @@ function watch(){

 module.exports = {
  watch               : watch,
-  build               : build
+  build               : build,


Expected property shorthand object-shorthand

houndci-bot · 2017-02-07T18:19:13Z

client/js/_player/components/common/recorder_timer.spec.js

+    props = {
+      timeout: 100,
+    };
+    result = TestUtils.renderIntoDocument(<RecorderTimer {...props} />);


JSX not allowed in files with extension '.js' react/jsx-filename-extension

You will want to rename this to recorder_timer.spec.jsx for the jsx syntax

Update our master to synch with atomic jolt

resource11 · 2017-02-07T20:38:20Z

OK, updates made @BenHSmith13. Can you review again?

coveralls · 2017-02-07T20:39:28Z

Coverage increased (+0.004%) to 99.352% when pulling cd4ff39 on CLIxIndia-Dev:feature/km/add-recorder-timer into bfbd87d on atomicjolt:master.

BenHSmith13 · 2017-02-07T22:55:13Z

You will want to rename this to recorder_timer.spec.jsx for the jsx syntax

BenHSmith13 · 2017-02-07T22:55:52Z

Also, the stuff Justin added that fixed the css caused the specs to break. I fixed it in the PR I just opened

Update our master branch with Atomic Jolt's master branch

Update CLIxIndia-Dev master to sync with latest Atomic Jolt master

coveralls · 2017-02-10T15:27:26Z

Coverage increased (+0.02%) to 99.368% when pulling dee6651 on CLIxIndia-Dev:feature/km/add-recorder-timer into bfbd87d on atomicjolt:master.

re-sync CLIxIndia-Dev master with Atomic Jolt master

coveralls · 2017-02-10T21:11:50Z

Coverage increased (+0.002%) to 99.364% when pulling 45d6643 on CLIxIndia-Dev:feature/km/add-recorder-timer into 4ff5a24 on atomicjolt:master.

sync CLIx-dev master w Atomic Jolt master

coveralls · 2017-02-14T20:50:39Z

Coverage increased (+0.003%) to 99.364% when pulling 6517b83 on CLIxIndia-Dev:feature/km/add-recorder-timer into 8892c54 on atomicjolt:master.

coveralls · 2017-02-14T20:53:30Z

Coverage increased (+0.003%) to 99.364% when pulling 6517b83 on CLIxIndia-Dev:feature/km/add-recorder-timer into 8892c54 on atomicjolt:master.

sync CLIx fork with Atomic Fork

coveralls · 2017-02-15T20:13:55Z

Coverage increased (+0.003%) to 99.364% when pulling e95035c on CLIxIndia-Dev:feature/km/add-recorder-timer into 8a654fd on atomicjolt:master.

coveralls · 2017-02-16T18:16:08Z

Coverage increased (+0.007%) to 99.368% when pulling d459d59 on CLIxIndia-Dev:feature/km/add-recorder-timer into 8a654fd on atomicjolt:master.

Update our master with atomic jolt master

* adds recorder timer, mods ui, passes tests * code QC pass - makes handlers reg vs lambda fns * Moves text into localizedString, lints code. * Adds localizedString dependency to spec file, defines test prop scenario

coveralls · 2017-02-16T19:17:13Z

Coverage increased (+0.003%) to 99.368% when pulling 20fdbf1 on CLIxIndia-Dev:feature/km/add-recorder-timer into 47e35d8 on atomicjolt:master.

coveralls · 2017-02-16T19:21:06Z

Coverage increased (+0.003%) to 99.368% when pulling 786dd8c on CLIxIndia-Dev:feature/km/add-recorder-timer into 47e35d8 on atomicjolt:master.

resource11 and others added 5 commits January 27, 2017 15:42

initial commit

976b732

adds yarn.lock

3dc64c5

Merge branch 'master' of https://github.com/atomicjolt/open_assessments

ab8a63a

add back the example .env file

52686c7

Merge branch 'master' of https://github.com/atomicjolt/open_assessments

d6c6710

houndci-bot reviewed Feb 3, 2017

View reviewed changes

client/styles/components/_components.answers.scss Outdated

}

50%{

opacity: 0;

}

Copy link

houndci-bot Feb 3, 2017

Choose a reason for hiding this comment

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

Rule declaration should be followed by an empty line

houndci-bot reviewed Feb 3, 2017

View reviewed changes

client/styles/components/_components.answers.scss Outdated

&.c-btn--stop{

Copy link

houndci-bot Feb 3, 2017

Choose a reason for hiding this comment

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

Opening curly brace { should be preceded by one space

houndci-bot reviewed Feb 3, 2017

View reviewed changes

BenHSmith13 reviewed Feb 3, 2017

View reviewed changes

houndci-bot reviewed Feb 7, 2017

View reviewed changes

Merge pull request #1 from atomicjolt/master

59d1c92

Update our master to synch with atomic jolt

resource11 force-pushed the feature/km/add-recorder-timer branch 2 times, most recently from ab38bd3 to cd4ff39 Compare February 7, 2017 20:36

cjshawMIT added 2 commits February 9, 2017 14:41

Merge pull request #2 from atomicjolt/master

c3c4d03

Update our master branch with Atomic Jolt's master branch

Merge pull request #3 from atomicjolt/master

135d4e9

Update CLIxIndia-Dev master to sync with latest Atomic Jolt master

Merge pull request #4 from atomicjolt/master

a9eacb2

re-sync CLIxIndia-Dev master with Atomic Jolt master

Merge pull request #7 from atomicjolt/master

66e3a4b

sync CLIx-dev master w Atomic Jolt master

resource11 force-pushed the feature/km/add-recorder-timer branch 3 times, most recently from 144bee1 to 6517b83 Compare February 14, 2017 20:50

Merge pull request #8 from atomicjolt/master

3b209c5

sync CLIx fork with Atomic Fork

resource11 force-pushed the feature/km/add-recorder-timer branch from 6517b83 to e95035c Compare February 15, 2017 20:09

Merge pull request #10 from atomicjolt/master

4775253

Update our master with atomic jolt master

resource11 force-pushed the feature/km/add-recorder-timer branch from 669adfc to b21626a Compare February 16, 2017 18:52

resource11 and others added 4 commits February 16, 2017 13:58

Feature/km/add recorder timer (#9)

eb20964

* adds recorder timer, mods ui, passes tests * code QC pass - makes handlers reg vs lambda fns * Moves text into localizedString, lints code. * Adds localizedString dependency to spec file, defines test prop scenario

adds recorder timer, mods ui, passes tests

5ee2b5c

Adds localizedString dependency to spec file, defines test prop scenario

ce3e313

removes stray console.log

786dd8c

resource11 force-pushed the feature/km/add-recorder-timer branch from 20fdbf1 to 786dd8c Compare February 16, 2017 19:18

resource11 closed this Feb 16, 2017

resource11 deleted the feature/km/add-recorder-timer branch February 16, 2017 19:28

Feature/km/add recorder timer #116

Feature/km/add recorder timer #116

Conversation

resource11 commented Feb 3, 2017

Code-review checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 3, 2017

BenHSmith13 Feb 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHSmith13 commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resource11 commented Feb 7, 2017

coveralls commented Feb 7, 2017

BenHSmith13 commented Feb 7, 2017

BenHSmith13 commented Feb 7, 2017

coveralls commented Feb 10, 2017

coveralls commented Feb 10, 2017

coveralls commented Feb 14, 2017

coveralls commented Feb 14, 2017

coveralls commented Feb 15, 2017

coveralls commented Feb 16, 2017

coveralls commented Feb 16, 2017

coveralls commented Feb 16, 2017

BenHSmith13 Feb 3, 2017 •

edited