CLDR-18220 Dashboard Other category for all other paths #4273
Conversation
- Adds a new notification category 'other'. It's not included in the default choice sets. - The query param includeOther=true adds this category. - UI in the dashboard makes the Other category look like similar categories, except that internally it reloads with the query param set/unset. (This reduces load in the normal case.) The benefit here is especially in being able to download a spreadsheet of all values in a coverage level.
| ; | ||
|
|
||
| other('O', "Other", "All other values"); |
There was a problem hiding this comment.
Since it's without errors, warnings and it isn't abstained, that this group should be thought of as "voted, no issues"? To keep the name minimal we can call it other, but I wonder if we should make it clearer.
There was a problem hiding this comment.
- It's not 'voted' if the user's logged out.
- How about just "No Issues"?
| @@ -551,7 +561,7 @@ public Response getParticipationFor( | |||
| coverageLevel = org.unicode.cldr.util.Level.fromString(level); | |||
| } | |||
| ReviewOutput reviewOutput = | |||
| new Dashboard().get(loc, target, coverageLevel, null /* xpath */); | |||
| new Dashboard().get(loc, target, coverageLevel, null /* xpath */, false); | |||
There was a problem hiding this comment.
nit: never add a boolean without a comment what it is
| new Dashboard().get(loc, target, coverageLevel, null /* xpath */, false); | |
| new Dashboard().get(loc, target, coverageLevel, null /* xpath */, false /* includeOther */); |
There was a problem hiding this comment.
I'm used to getting that filled in by the IDE but sounds good
There was a problem hiding this comment.
Ah! If you have a moment during the Stanford visit I'd love for your help setting up my IDE to for things like this. Do you use Eclipse or VSCode?
There was a problem hiding this comment.
VS code with the red hat, Java package
| @@ -402,7 +402,7 @@ private static Dashboard.ReviewNotification[] getOnePathDash( | |||
| String baseXp, CLDRLocale locale, CookieSession mySession) { | |||
| Level coverageLevel = Level.get(mySession.getEffectiveCoverageLevel(locale.toString())); | |||
| Dashboard.ReviewOutput ro = | |||
| new Dashboard().get(locale, mySession.user, coverageLevel, baseXp); | |||
| new Dashboard().get(locale, mySession.user, coverageLevel, baseXp, false); | |||
There was a problem hiding this comment.
Same nit
| new Dashboard().get(locale, mySession.user, coverageLevel, baseXp, false); | |
| new Dashboard().get(locale, mySession.user, coverageLevel, baseXp, false /* includeOther */); |
There was a problem hiding this comment.
This seems like a very useful feature for those who need it, when they need it. On the other hand it has a bit of a "creeping featurism" aspect to it, complicating the interface with something that most (?) users might not use. Is the benefit mainly for downloading, rather than scrolling in the Dashboard itself? If so, it might make sense to apply it only to downloading, and make it an option in the download interface only. Performance may also be a concern, especially if it takes a long time to update the dashboard after a user clicks on the checkbox when they're merely curious to see what happens
| class="cldr-nav-btn" | ||
| @click="reloadIncludeOther" | ||
| > | ||
| <input type="checkbox" /> Other |
There was a problem hiding this comment.
suggestion for future improvement: hover text for this control with brief explanation what it does, e.g., "Include notifications for ALL paths within the chosen coverage level that do not belong to another notification category. Note: the number of 'other' notifications may be very large"
That's an idea. The Download button currently operates off the data you've already downloaded, BUT, what we could do is have it always do a fresh download (wouldn't be too bad…) Suggestion is to merge this for today (applying suggestions… for this week) and review if need be. I think it stays out of the way of the normal case |
CLDR-18220
This PR completes the ticket.
Adds a new notification category 'other'. It's not included in the default choice sets.
The query param includeOther=true adds this category.
UI in the dashboard makes the Other category look like similar categories, except that
internally it reloads with the query param set/unset. (This reduces load in the normal case.)
The benefit here is especially in being able to download a spreadsheet of all values in a coverage
level.
UI
Unchecked
Checked
Downloaded
Dash_csw_default(2).xlsx
ALLOW_MANY_COMMITS=true