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.

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

Assessment UX Re-Design: Progress Bubble Tooltip #27862

Merged
merged 3 commits into from Apr 4, 2019

Conversation

dmcavoy
Copy link
Contributor

@dmcavoy dmcavoy commented Apr 3, 2019

When in the mini rubric experiment and you hover over a level that is marked as an assessment you will see the check-circle icon in addition to the other icon for that level in the tooltip.

Before

Screen Shot 2019-04-03 at 3 36 01 PM

After

In Experiment

Screen Shot 2019-04-03 at 3 35 05 PM

Not in Experiment

Screen Shot 2019-04-03 at 3 35 25 PM

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Code and tests look good!

Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

really great work, dani! i left two small nit comments, but neither are blockers for merging

@@ -176,6 +178,7 @@ class ProgressBubble extends React.Component {
tooltipId={tooltipId}
icon={levelIcon}
text={tooltipText}
includeAssessmentIcon={levelIsAssessment}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since we don't consume the value of levelIsAssessment anywhere else, we can call isLevelAssessment(level) like this:

Suggested change
includeAssessmentIcon={levelIsAssessment}
includeAssessmentIcon={isLevelAssessment(level)}

and delete the assignment to levelIsAssessment on line 127

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I had that thought and then left it like this because I'm probably going to need this variable to do some of the other assessment ux work such as adding the little check-circle in the corner of the bubble.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, understandable! makes sense to leave it as-is then

@dmcavoy dmcavoy merged commit ecf3846 into staging Apr 4, 2019
@dmcavoy dmcavoy deleted the assessment-ux-tooltip branch April 4, 2019 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants