Move styles inline for React progress components #8756
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,103 @@ | ||
| import Radium from 'radium'; | ||
| import React from 'react'; | ||
| import { STAGE_PROGRESS_TYPE } from './types'; | ||
| import { saveAnswersAndNavigate } from '../../levels/saveAnswers'; | ||
| import color from '../../color'; | ||
|
|
||
| var style = { | ||
| overviewContainer: { | ||
| display: 'table-cell', | ||
| verticalAlign: 'middle', | ||
| paddingRight: '10px' | ||
| }, | ||
| headerContainer: { | ||
| padding: '5px 8px', | ||
| backgroundColor: color.lightest_gray, | ||
| border: `1px solid ${color.lighter_gray}`, | ||
| borderRadius: '5px' | ||
| }, | ||
| dot: { | ||
| puzzle: { | ||
| display: 'inline-block', | ||
| width: '24px', | ||
|
There was a problem hiding this comment. would it make sense to use variables for some of these values that are repeated a few times? There was a problem hiding this comment. also many of them can be numbers instead of strings with px There was a problem hiding this comment. Unit-less numbers will always have 'px' appended, even things like There was a problem hiding this comment. Is there a shorthand for There was a problem hiding this comment. Anything not in the list from your link will automatically get px. The one that's gotten me a few times is idk about shorthand. Could try There was a problem hiding this comment. Good catch on
It makes me a little nervous that this mistake is so easy to make. I almost prefer always including units to avoid ambiguity, but I see the benefits of being able to do math with the shorthand. |
||
| height: '24px', | ||
| fontSize: '14px', | ||
| textAlign: 'center', | ||
| lineHeight: '24px', | ||
| borderRadius: '24px', | ||
| borderWidth: '2px', | ||
| borderStyle: 'solid', | ||
| borderColor: color.lighter_gray, | ||
| margin: '0 -1px', | ||
| transition: 'background-color .2s ease-out, border-color .2s ease-out, color .2s ease-out', | ||
| ':hover': { | ||
| textDecoration: 'none', | ||
| color: color.white, | ||
| backgroundColor: color.level_current | ||
| } | ||
| }, | ||
| unplugged: { | ||
| width: 'auto', | ||
| fontSize: '13px', | ||
| padding: '0 10px' | ||
| }, | ||
| assessment: { | ||
| borderColor: color.assessment | ||
| }, | ||
| small: { | ||
| width: '7px', | ||
| height: '7px', | ||
| borderRadius: '7px', | ||
| lineHeight: 'inherit', | ||
| fontSize: 0 | ||
| }, | ||
| overview: { | ||
| height: '30px', | ||
| width: '30px', | ||
| margin: '2px', | ||
| fontSize: '16px', | ||
| lineHeight: '32px' | ||
| } | ||
| }, | ||
| status: { | ||
| submitted: { | ||
| color: color.white, | ||
| backgroundColor: color.level_submitted | ||
| }, | ||
| perfect: { | ||
| color: color.white, | ||
| backgroundColor: color.level_perfect | ||
| }, | ||
| passed: { | ||
| color: color.white, | ||
| backgroundColor: color.level_passed | ||
| }, | ||
| attempted: { | ||
| color: color.charcoal, | ||
| backgroundColor: color.level_attempted | ||
| }, | ||
| not_tried: { | ||
| color: color.charcoal, | ||
| backgroundColor: color.level_not_tried | ||
| }, | ||
| review_rejected: { | ||
| color: color.white, | ||
| backgroundColor: color.level_review_rejected | ||
| }, | ||
| review_accepted: { | ||
| color: color.white, | ||
| backgroundColor: color.level_review_accepted | ||
| } | ||
| } | ||
| }; | ||
|
There was a problem hiding this comment. the neat thing about having these styles in .js is that we can hoist them up to a more common component if we end up using them elsewhere. There was a problem hiding this comment. though it is amusing to see the end of .css/.scss and the return of putting styles near the rendering itself. There was a problem hiding this comment. Also wonder if dot has enough complexity that it deserves its own component? There was a problem hiding this comment. Extracted a There was a problem hiding this comment. Re: CSS, I miss the "Cascading" part of Cascading Style Sheets. It used to be that we could set one class on a root element and easily apply a totally different set of styles for many elements (e.g. I like that components own all their styles and can be included with no extra dependencies, but it seems like we've lost some of the power of CSS. Outside CSS can still have unintended effects on components. Maybe Shadow DOM will make this better in the future? |
||
|
|
||
| /** | ||
| * Stage progress component used in level header and course overview. | ||
| */ | ||
| var StageProgress = React.createClass({ | ||
| propTypes: { | ||
| levels: STAGE_PROGRESS_TYPE, | ||
| currentLevelId: React.PropTypes.string, | ||
| largeDots: React.PropTypes.bool, | ||
| saveAnswersFirst: React.PropTypes.bool.isRequired | ||
| }, | ||
|
|
@@ -20,24 +109,26 @@ var StageProgress = React.createClass({ | |
| }, | ||
|
|
||
| render() { | ||
| var progressDots = this.props.levels.map(level => { | ||
| var uid = level.uid || level.id.toString(); | ||
|
|
||
| var dotStyle = Object.assign({}, style.dot.puzzle); | ||
| if (level.kind === 'assessment') { | ||
| Object.assign(dotStyle, style.dot.assessment); | ||
| } | ||
|
|
||
| if (this.props.largeDots) { | ||
|
There was a problem hiding this comment. maybe a comment somewhere explaining what large dots vs. small dots are? must admit i'm not quite sure what the difference is. |
||
| Object.assign(dotStyle, style.dot.overview); | ||
| if (uid === this.props.currentLevelId) { | ||
| Object.assign(dotStyle, {borderColor: color.level_current}); | ||
| } | ||
| } else if (uid !== this.props.currentLevelId) { | ||
| Object.assign(dotStyle, style.dot.small); | ||
| } | ||
|
|
||
| var isUnplugged = isNaN(level.title); | ||
| if (isUnplugged) { | ||
| Object.assign(dotStyle, style.dot.unplugged); | ||
| } | ||
|
|
||
| var onClick = null; | ||
|
|
@@ -47,24 +138,23 @@ var StageProgress = React.createClass({ | |
| } | ||
|
|
||
| return ([ | ||
| <a | ||
| key={uid} | ||
| className={`level-${level.id}`} | ||
| href={level.url} | ||
| onClick={onClick} | ||
| style={[dotStyle, style.status[level.status || 'not_tried']]}> | ||
| {level.title} | ||
| </a>, | ||
| ' ' | ||
| ]); | ||
| }); | ||
|
|
||
| return ( | ||
| <div className='react_stage' style={this.props.largeDots ? style.overviewContainer : style.headerContainer}> | ||
|
There was a problem hiding this comment. Is overview vs. header the difference between drop-down view and in-header stage view, or something else? There was a problem hiding this comment. Yep! I'll rename these to something better. |
||
| {progressDots} | ||
| </div> | ||
| ); | ||
| } | ||
| }); | ||
| module.exports = Radium(StageProgress); | ||
|
There was a problem hiding this comment. could use const/let vs. var throughout (tho not a requirement that you change it) There was a problem hiding this comment. Fixed in c149942cf4e47f8b6c9508cc71a2d89bb899558a. |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, dumb question.. is it normal to use these kinds of custom tags in react?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@breville This is how you pass props to child components (i.e. The CourseProgressRow component will now have a prop named
currentLevelIdset to the same value as this component'scurrentLevelId