Show icons in redesigned and existing progress dots (Unrevert 13212) #13248
Conversation
dashboard/app/models/script_level.rb
Outdated
| elsif assessment | ||
| 'fa-list-ol' | ||
| end | ||
| end |
There was a problem hiding this comment.
@joshlory FYI for overriding icon for assessments. Looking at this again, I think that this (a) deserves a comment that captures some of the discussion we've had around icons and (b) I probably should reverse the conditionals, i.e.
if assessment
'fa-list-ol'
else
level.icon
end
Even though there probably aren't currently any level types that have an icon, that we would also make an assessment, if some existing type later did provide a default icon, I think we'd still want assessment to override.
| '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', |
| @@ -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 | |||
There was a problem hiding this comment.
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]; |
There was a problem hiding this comment.
You used to return '' empty string in the fa-list-ol case; now you return undefined. Intentional?
| @@ -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); | |||
There was a problem hiding this comment.
Don't think it makes a big difference one way or the other really, but undefined seems slightly more accurate.
There was a problem hiding this comment.
Should your default case return undefined then?
There was a problem hiding this comment.
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.
Bjvanminnen
changed the title
#13212 was reverted because when our progressRedesign experiment was not enabled, assessment level were not showing up properly. This is because even tho
iconClassNameknew to ignore 'fa-list-ol', we also rendered differently based on whether or not we had alevel.icon.This unifies the logic a little bit more, and gets us closer to the place I'd like us to be where the server sends us an IconType instead of a className. I'm essentially pretending that's what it does, except that the IconType key is actually just the className (if that makes sense)