-
Notifications
You must be signed in to change notification settings - Fork 25
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
2280 add quick summary of num missions, distance, labels, validations, and accuracy to dashboard #2289
2280 add quick summary of num missions, distance, labels, validations, and accuracy to dashboard #2289
Conversation
but tabling for now until @misaugstad has a chance to work on some backend queries for accuracy, etc. (based on our recent IM discussion)
…abels, num validations
just commented them out for now as i don't think they're valuable/interesting to the user
…cy, and mission counts Added initial version of user dashboard with distance, labels, accuracy, and mission counts and iconography. Need to hook up accuracy to backend function. Also strips out old user dashboard stuff that's no longer relevant.
added getUserAccuracy to LabelValidationTable and tried to hook it up to userProfile.scala.html but it's failing
Still not fully working, however, see: #1363 (comment)
New code line: @LabelValidationTable.getUserAccuracy(user.get.userId).map(a => "$%.1f%%".format(a * 100)).getOrElse("N/A") Looks like this: #1363 (comment)
…Webpage into 1680-give-users-more-feedback-on-dashboard
Added validations as discussed #2280 (comment)
Love it!!! So exciting |
Since we are no longer graphing a timeline and doing other things, we may be able to get rid of some of the many JavaScript imports (none of which are commented, so I don't know what they're associated with in the code).
We need to keep the LabelMap related code, of course, which I believe is all here:
|
Related to that, I just took a quick peek. And you removed the HTML elements for the audit count chart and the missions list, but you didn't remove the JavaScript that filled in that chart and table in Progress.js. We need to delete all that code that we're no longer using. And I believe that both the audit count chart and the missions list had endpoints created to get that data, so we should be able to delete all the code controller and model code that is used only for those things. |
And for those imports, I imagine that we can probably get rid of d3, c3, and moment. But the i18next imports are for Spanish translations, and Progress includes all the JavaScript code for the page. Underscore just adds nice functional programming helper functions, but I think the only place we happen to be using them right now is for the missions list chart, so that can probably go too. |
TBH, |
Agree that Progress.js should be renamed to UserDashboard.js or the like. But I do think that any time an HTML element is deleted, we should be searching the entire project for the div IDs and associated CSS class names and deleting the associated CSS/JS that isn't used anywhere else. I can still do that for this PR, but I'm mostly saying it because it is easy for either of us to forget to do that when coding or reviewing, so if both of us are trying to remember to do that, hopefully at least one of us remembers every time it happens 🙃 I should also add that to the PR template! |
Sure. Totally agree. Let me know if you want me to do it. Otherwise, I'm gonna focus on #2281 with the goal of creating a PR tomorrow. As mentioned during our 1:1, I won't have as much time the rest of the quarter to dev as heavily as I've been the past 1.5 weeks. So, trying to push on #2281 now that I've created this PR. |
Yep, please go for it! Just know that it'll take me some time to get through all the open PRs you made 😁 but it'll all get closed out in the coming weeks |
Gotcha. Hopefully this one won't take a couple of weeks as it's relatively straightforward and you helped with the backend stuff. Most of the heavy lifting here is just in coming up with a clean design, making the icons, etc. I think it would be nice to merge this PR alongside @rpechuk's leaderboard work as they complement each other. |
Also note that we still have this (for when the user is a turker):
Related to this, is there any reason why a turker would want to see the timeline chart of audit activity or the table? which corresponds to:
|
As per our discussion here (#2289 (comment)), I removed outdated JavaScript code from Progress.js that is no longer relevant given the user dashboard redesign Also added in a conditional for this: document.getElementById("td-total-distance-audited") which will only exist when the user is a turker
Per our discussion (#2289 (comment)), removed all unnecessary JavaScript libraries from userProfile.scala.html, including c3, d3, and moment. I also removed underscore.js
No I don't think there's a reason they would need/want to see that. |
Ok. Good. Then I think we are ready again for the PR.
…Sent from my iPhone
On Oct 1, 2020, at 5:42 PM, Mikey Saugstad ***@***.***> wrote:
No I don't think there's a reason they would need/want to see that.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
Cool. Reminder to myself when I look at this to remove the routes/controller/model code associated with the ajax calls removed from Progress.js. |
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'm going to add comments throughout the code as I make changes just so that you know what I've been changing, but I'll likely just do most/all of it myself instead of asking you to fix stuff.
@@ -135,6 +137,48 @@ object LabelValidationTable { | |||
} | |||
} | |||
|
|||
/** | |||
* Calculates and returns the user accuracy for the supplied userId. The accuracy calculation is performed |
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.
Our max line length (defined in the style guide) is 120 characters so these comments can extend a bit further.
*/ | ||
def getUserAccuracy(userId: UUID): Option[Float] = db.withSession { implicit session => | ||
val accuracyQuery = Q.query[String, Option[Float]]( | ||
"""SELECT CASE WHEN validated_count > 9 THEN accuracy ELSE NULL END AS accuracy |
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.
Make sure to change your settings to use spaces instead of tabs, as per style guide.
@@ -238,6 +282,16 @@ object LabelValidationTable { | |||
.size.run | |||
} | |||
|
|||
/** | |||
* Counts the number of validations performed by this user (given the supplied userId) |
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.
Include periods at the end of comments, as per style guide.
public/stylesheets/userProfile.css
Outdated
font-family: raleway-extrabold, sans-serif; | ||
} | ||
|
||
.ps-skyline-stats-holder-stat-number { |
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.
Fixed the name of this and just updated the names of some other CSS in here.
app/views/userProfile.scala.html
Outdated
<span class="primary-user-stats-holder-stat-header">@Messages("dashboard.validations")</span> | ||
<span class="primary-user-stats-holder-stat-header">@Messages("dashboard.accuracy")</span> | ||
|
||
<img src='@routes.Assets.at("images/icons/project_sidewalk_flag.png")'/> |
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.
Adding alt text to the images for screen readers and such
</table> | ||
</div> | ||
<div class="row"> | ||
<table class="table table-striped table-condensed"> |
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.
Some of the CSS from removed HTML elements can be removed too
Addresses #2280 (and parts of the very old #1363) by:
English
Old
New
Spanish
Old
New
Other
I also tested it as a brand new user and the dashboard properly showed me zeros and N/As, as appropriate. Here's a screenshot after validating 10 things (and completing 1 mission):