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.

Sign up for GitHub

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

Jump to bottom

Show icons in redesigned and existing progress dots (Unrevert 13212) #13248

Merged
merged 4 commits into from
Feb 15, 2017
Merged

Show icons in redesigned and existing progress dots (Unrevert 13212) #13248

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
378b82a
Revert "Merge pull request #13243 from code-dot-org/revert-13212-prog…
Bjvanminnen Feb 14, 2017
2e10d3e
treat level.icon as a type to be mapped to a className
Bjvanminnen Feb 14, 2017
05ac9b7
add comment and fix conditional
Bjvanminnen Feb 15, 2017
92b3d5f
[skip ci] remove old comment
Bjvanminnen Feb 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 16 additions & 3 deletions apps/src/code-studio/components/progress/ProgressDot.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,19 @@ const styles = {
status: BUBBLE_COLORS
};

// Longer term, I'd like the server to provide us an icon type, instead of a
// className. For now, I'm going to use the className in level.icon as if it
// were actually a type key.
const iconClassFromIconType = {
'fa-file-text': 'fa fa-file-text',
// Explicitly don't want to use an icon for this type
'fa-list-ol': undefined,
'fa-external-link-square': 'fa fa-external-link-square',
'fa-video-camera': 'fa fa-video-camera',
'fa-stop-circle': 'fa fa-stop-circle',
'fa-map': 'fa fa-map',
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

};

export const BubbleInterior = React.createClass({
propTypes: {
showingIcon: React.PropTypes.bool,
Expand Down Expand Up @@ -202,8 +215,8 @@ export const ProgressDot = Radium(React.createClass({

// fa-list-ol is used only by our redesigned dots, but we don't want to use
// it here
Copy link
Contributor

Choose a reason for hiding this comment

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

The fa-list-ol special case comment here should maybe be moved up to your mapping.

if (level.icon && level.icon !== 'fa-list-ol') {
return 'fa ' + level.icon;
if (level.icon) {
return iconClassFromIconType[level.icon];
Copy link
Contributor

Choose a reason for hiding this comment

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

You used to return '' empty string in the fa-list-ol case; now you return undefined. Intentional?

}
return '';
},
Expand Down Expand Up @@ -235,7 +248,7 @@ export const ProgressDot = Radium(React.createClass({
isLocked && styles.disabledLevel
]}
>
{(level.icon && !isPeerReview) ?
{(iconClassFromIconType[level.icon] && !isPeerReview) ?
<i
className={this.iconClassName()}
style={[
Expand Down
3 changes: 2 additions & 1 deletion apps/test/unit/code-studio/components/progress/ProgressDotTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,8 @@ describe('ProgressDot component tests', () => {
);

const result = renderer.getRenderOutput();
expect(result.props.children[0].props.className).to.equal('');
expect(result.props.children[0].type).to.equal('div');
expect(result.props.children[0].props.className).to.equal(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, seems intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't think it makes a big difference one way or the other really, but undefined seems slightly more accurate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should your default case return undefined then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh yes, probably. I expect to do some future work here in the near future, so I'll try to remember to clean that up then.

});

it('has no icon in header', () => {
Expand Down