-
Notifications
You must be signed in to change notification settings - Fork 159
Visual update of scores on analysis tab. #1419
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
Conversation
I really like the bar-type visualization! However, the bars themselves seems a bit too "fat", and I think would benefit from a more light visualization. I also don't think we use rounded corners anywhere else, so that looks a bit off. I think the overall score bar should be colored to reflect the score, like in thee examples, but also think we can use the same color for the other three bars. |
Any suggestion on the color (or combination of them)? |
ad330ee
to
87ced31
Compare
@mit-mit PTAL. Attached two image, and currently deploying to staging (should be available in ~10-20 minutes). |
Can you try the slimmer bar layout I mentioned above? Something similar to what Material specs: That would also allow us to use a regular font for the score (it would be placed either above or after the bar) |
Oh, and a nit: Using the theme (blue) color for the tab selection -- the blue line under (22) -- looks odd now that we use use colors for the score. Can we try to use a black/grayscale color for that? |
@mit-mit: I was already considering these pointers, but there is a question with them: if the score is high, the label above the progressbar can fit in the same line as the score title. However, if the value is low, it will either:
I couldn't really decide on those, that's why I've opted for the current bar. Any preference on the above? |
9c484c8
to
e9ac509
Compare
That looks great! One small tweak: I think we should remove the small lines you have at each end of the bar. The Material spec doesn't use those. |
f64366b
to
9507dd5
Compare
That looks great, design LGTM @sortie can you review the code? |
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.
lgtm
app/lib/frontend/color.dart
Outdated
: 'rgba($r, $g, $b, ${a.toStringAsFixed(4)})'; | ||
} | ||
|
||
class Brush { |
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.
Should this be in its own file?
Is this a generic thing or just for the progress bars and scores? Is this also used for the circles with scores? A little dartdoc with such details could maybe be nice.
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.
I think this file should become a go-to place for all color-related code, I don't intend to introduce a full-blown color-handling library for other clients. Adding dartdoc to make the intent better on these classes.
app/lib/frontend/color.dart
Outdated
} | ||
} | ||
|
||
final _blackShadow = Color.black.change(a: 0.5); |
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.
These are more presentational logic that doesn't need to be together with the low level Color and Brush class declarations.
These can be const. _blackShadow can also be const if you spell it out, or have a const Color.withChange(const color, {int r, int g, int b}) constructor.
Can you comment what the name of these colors are?
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.
Unless there is a big benefit of doing the Color.withChange
constructor, I'd rather stick with the shorter change
method. I don't have much reason behind it, except that it is closer to e.g. how the Uri
works. Maybe change
could be renamed to replace
to follow that pattern?
app/lib/frontend/color.dart
Outdated
final _defaultBrush = new Brush( | ||
background: _scoreBoxMissing, color: Color.white, shadow: _blackShadow); | ||
|
||
Brush overallScoreBrush(double score) { |
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.
Perhaps add some dart doc. The above is plain old data but now we're in logic code.
Map<String, dynamic> _renderScoreBars(AnalysisExtract extract) { | ||
String renderScoreBar(double score, Brush brush) { | ||
return _renderTemplate('pkg/score_bar', { | ||
'percent': _formatScore(score ?? 0.0), |
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.
A skipped package will have a 0 score rather than a N/A score?
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.
The percent
will be used only to render the width of the progress bar, while the score
one line below will be used to render the actual text label. Btw. great catch, because I haven't used the score
to do so.
@@ -835,10 +861,12 @@ String _getAuthorsHtml(List<String> authors) { | |||
}).join('<br/>'); | |||
} | |||
|
|||
bool _isAnalysisSkipped(AnalysisStatus status) => |
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.
Maybe skipped is the wrong word here? I was thinking of packages that are brand new (no data yet), or there's a minor problem why the data isn't there right now; not so much that it's packages that are obsolete.
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.
We don't have an AnalysisStatus
value for the pending ones (e.g. the one that is scheduled but not done yet), only for the ones that have been done. This way if a status is missing (==null), then it is by default pending (and should be scheduled), otherwise it is done.
We 'skip' two kinds of packages in the analysis: one that is older than two years, and one that is flagged as discontinued. We shall analyse everything else.
Ideally #1352 could also track the scheduled but not done yet status too.
9507dd5
to
5260825
Compare
5260825
to
9453c2d
Compare
Changed to "progress bar"-like visual layout, intentionally with muted colors (choosing colors is hard). Preview at the end of the description, and also deployed to staging, a few examples: 100%, 77%, 50%, 22%, outdated.
This could go in multiple directions:
In the first case, we could restructure the score circle to have similar color indicator bars, in the second I believe the circle should match the color of the overall score's color here.
/cc @mit-mit @sortie: how shall we balance the visuals with the new icons?