Move styles inline for React progress components #8756

joshlory · 2016-06-03T21:02:24Z

The change moves the CSS styles inline for the React progress components. I've updated the UI tests to check CSS color instead of class name.

There's still one external jQuery dependency on the class name of the level dots (in loadApp.js). I plan to move this to use the Redux store in a separate PR.

[test ui]

joshlory · 2016-06-03T21:03:50Z

No rush on this PR! I'm OOF today and won't be able to look at feedback until Monday.

breville · 2016-06-03T22:36:01Z

code-studio/src/js/components/progress/course_progress.jsx

    display: React.PropTypes.oneOf(['dots', 'list']).isRequired,
    stages: React.PropTypes.arrayOf(STAGE_TYPE)
  },

  getRow(stage) {
    if (this.props.display === 'dots') {
-      return <CourseProgressRow stage={stage} key={stage.name} />;
+      return <CourseProgressRow stage={stage} key={stage.name} currentLevelId={this.props.currentLevelId} />;


sorry, dumb question.. is it normal to use these kinds of custom tags in react?

@breville This is how you pass props to child components (i.e. The CourseProgressRow component will now have a prop named currentLevelId set to the same value as this component's currentLevelId

breville · 2016-06-03T22:55:42Z

looks great to me. react stuff is still new to me, but didn't see any issues.

Bjvanminnen · 2016-06-03T22:58:17Z

code-studio/src/js/components/progress/course_progress_row.jsx

@@ -2,29 +2,47 @@
 import React from 'react';
 import { STAGE_TYPE } from './types';
 import StageProgress from './stage_progress.jsx';
+import color from '../../color';
+
+var style = {


nit: could be const. also our convention has been to call this variable styles rather than style.

Fixed in c149942cf4e47f8b6c9508cc71a2d89bb899558a.

Bjvanminnen · 2016-06-03T23:03:38Z

couple nits, but generally looks good

joshlory · 2016-06-07T00:06:28Z

@Bjvanminnen ready for another look when you get a chance.

Bjvanminnen · 2016-06-07T00:08:36Z

code-studio/src/js/components/progress/course_progress_row.jsx

@@ -2,29 +2,47 @@
 import React from 'react';
 import { STAGE_TYPE } from './types';
 import StageProgress from './stage_progress.jsx';
+import color from '../../color';
+
+const STYLES = {


super nit, but our convention thus far is to have this be a lower case styles

Bjvanminnen · 2016-06-07T00:10:45Z

code-studio/src/js/components/progress/stage_progress.jsx

+        largeDots={this.props.largeDots}
+        saveAnswersFirst={this.props.saveAnswersFirst}
+      />, ' '
+    ]));


You're mapping each to an array of ProgressDot? I think you can get rid of the brackets and just map each to a ProgressDot

In order to get the exact Code Studio™ spacing in the header we actually need a space between the elements 😄. I tried to recreate it with margin but it's not a whole number of pixels, and seems to round differently. I can look at getting rid of the ' ' in a future PR.

Ahh. missed the space. Makes sense.

Gif showing the difference with margin vs. spacing from white space:

White spacing:

Margin:

I think the diff is acceptable so I'll go ahead and remove ' ' and add the margin. It may be a small enough change to not trigger an Eyes test failure.

Bjvanminnen · 2016-06-07T00:13:01Z

Couple of high level nits (I didn't comment on every instance)
styles vs STYLES. I don't have a great argument as to why, other than this is what we've been doing elsewhere.
A bunch of places you're using let where you could be using const. Preference should be to use const anywhere you can, as it makes it easier to know which variables you have to worry about when thinking about what is/isnt mutable.

joshlory · 2016-06-07T00:20:00Z

Ah you're totally right, the Airbnb style guide we reference recommends lowercase const names.

See: http://stackoverflow.com/a/8394652

Bjvanminnen · 2016-06-07T17:47:31Z

code-studio/src/js/components/progress/course_progress_row.jsx

 import StageProgress from './stage_progress.jsx';
+import color from '../../color';
+
+const STYLES = {


Bjvanminnen · 2016-06-07T17:48:17Z

looks like there's still one instance of STYLES vs. styles, otherwise lgtm

2230104 Merge pull request #8780 from code-dot-org/npm-piskel (Brad Buchanan) 69e5430 Merge branch 'test' into staging (Josh Lory) d9c4601 Merge branch 'production' into test (Josh Lory) a895055 Merge pull request #8811 from code-dot-org/test (Josh Lory) 0585f54 Merge pull request #8783 from code-dot-org/pcardune-content-security-policy (Paul Carduner) e873ec7 Merge pull request #8786 from code-dot-org/pcardune-apps-coverage-report (Paul Carduner) 2d57ffd Merge pull request #8809 from code-dot-org/revert-8748-firebase-basics (David Bailey) 0e85abc Revert "Firebase basics" (David Bailey) bdf3fbb Merge pull request #8784 from code-dot-org/hide_solution_link_for_pd (Mehal Shah) c274d51 Merge pull request #8808 from code-dot-org/hotfix-progress-dots (Josh Lory) 3fda3ff Hotfix: fix unplugged progress dots rendering issue (Josh Lory) e1bcd99 Amend 799c13c (Josh Lory) bfde939 Merge pull request #8787 from code-dot-org/bounce-ui-tests (David Bailey) 460c2d6 Merge pull request #8748 from code-dot-org/firebase-basics (David Bailey) 85bfdce Merge pull request #8794 from code-dot-org/fixPrivilegesUI (ashercodeorg) fa1d792 Merge pull request #8788 from code-dot-org/fix-test-iphone-hocReset (Andrew Oberhardt) 0ae0e16 Merge pull request #8791 from code-dot-org/cucumber-logs-on-s3 (Brad Buchanan) d36fa89 Merge pull request #8806 from code-dot-org/test (Josh Lory) a644205 PR feedback - going back to using forbidden (Mehal Shah) fc95f09 Merge pull request #8804 from code-dot-org/staging (Josh Lory) c373dff Merge pull request #8803 from code-dot-org/progress-dots-fix (Josh Lory) 799c13c Unplugged dots should be small when not the current level (Josh Lory) 9baccd6 Merge pull request #8801 from code-dot-org/localsLevel (Brent Van Minnen) 6826eeb locals.level instead of just level (Brent Van Minnen) 5d82aee Merge remote-tracking branch 'origin/staging' into cucumber-logs-on-s3 [ui test] (Brad Buchanan) 267321f Extract log uploader from runner.rb (Brad Buchanan) 5a59be7 Merge pull request #8800 from code-dot-org/staging (Brent Van Minnen) 436eee6 Use branchname as S3 prefix for log (Brad Buchanan) ce5fd7a Merge pull request #8797 from code-dot-org/pcardune-fix-netsim (Brent Van Minnen) d48a4ad Use locals to refer to variables passed in (Paul Carduner) 1341eda Merge pull request #8756 from code-dot-org/react-progress-inline-styles (Josh Lory) f28869e Load Piskel via nonfingerprinted path (Brad Buchanan) 6d6e7d0 Code review feedback [ci skip] (Josh Lory) 76ad952 Merge pull request #8796 from code-dot-org/levelbuilder (Josh Lory) e53f5c2 Level builder changes (Continuous Integration) 5e53305 Add some more documentation (Paul Carduner)

joshlory added 6 commits June 1, 2016 15:17

Move styles inline for the progress React components

65e68b5

Fix Radium warning

a55cd4a

Inline status styles

041f58f

Make the orange current_puzzle circle work with LevelGroup

c7cfd34

Update UI tests to check progress via color instead of class name

c92e97f

Add back classes for async load (fixes UI tests)

96fc1d7

[test ui]

joshlory assigned Bjvanminnen and breville Jun 3, 2016

breville reviewed Jun 3, 2016
View reviewed changes

Bjvanminnen reviewed Jun 3, 2016
View reviewed changes

joshlory added 3 commits June 4, 2016 11:09

Update starwars UI test [test ui]

5841c42

Code review feedback

1b01f55

Extract ProgressDot React component

8e1efcc

joshlory force-pushed the react-progress-inline-styles branch from 97aad59 to 8e1efcc Compare June 6, 2016 23:22

Bjvanminnen reviewed Jun 7, 2016
View reviewed changes

Merge branch 'staging' into react-progress-inline-styles

98cbd22

joshlory added 3 commits June 6, 2016 17:46

Get rid of whitespace-based spacing and use margin

04b030b

See: http://stackoverflow.com/a/8394652

Lowercase JavaScript const names

4f0f20a

A level can occur multiple times in a script, ensure key is unique

a028c6b

Bjvanminnen reviewed Jun 7, 2016
View reviewed changes

Code review feedback [ci skip]

6d6e7d0

joshlory merged commit 1341eda into staging Jun 7, 2016

joshlory deleted the react-progress-inline-styles branch June 7, 2016 17:53

joshlory mentioned this pull request Jun 7, 2016

Unplugged dots should be small when not the current level #8803

Merged

joshlory mentioned this pull request Jun 8, 2016

Restyle PLC unit overview #8831

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move styles inline for React progress components #8756

Move styles inline for React progress components #8756

joshlory commented Jun 3, 2016

joshlory commented Jun 3, 2016

breville Jun 3, 2016

Bjvanminnen Jun 3, 2016

breville commented Jun 3, 2016

Bjvanminnen Jun 3, 2016

joshlory Jun 6, 2016

Bjvanminnen commented Jun 3, 2016

joshlory commented Jun 7, 2016

Bjvanminnen Jun 7, 2016

Bjvanminnen Jun 7, 2016

joshlory Jun 7, 2016

Bjvanminnen Jun 7, 2016

joshlory Jun 7, 2016

Bjvanminnen commented Jun 7, 2016

joshlory commented Jun 7, 2016

Bjvanminnen Jun 7, 2016

Bjvanminnen commented Jun 7, 2016

Navigation Menu

Move styles inline for React progress components #8756

Move styles inline for React progress components #8756

Conversation

joshlory commented Jun 3, 2016

joshlory commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breville commented Jun 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bjvanminnen commented Jun 3, 2016

joshlory commented Jun 7, 2016

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

Bjvanminnen commented Jun 7, 2016

joshlory commented Jun 7, 2016

Choose a reason for hiding this comment

Bjvanminnen commented Jun 7, 2016