Make PackageDownloadsDetail and PackageDownloadsByVersion graphs update without refreshing the page #3951
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scottbommarito
requested review from
joelverhagen,
agr,
loic-sharma,
xavierdecoster,
ryuyu,
chenriksson,
cristinamanum and
skofman1
skofman1
reviewed
May 11, 2017
| // | ||
| // GET: /stats/packages/{id}/{version} | ||
|
|
||
| public virtual async Task<ActionResult> PackageDownloadsDetail(string id, string version, string[] groupby) |
There was a problem hiding this comment.
please add UTs for the new endpoints.
skofman1
reviewed
May 11, 2017
| // | ||
| // GET: /stats/reports/packages/{id} | ||
|
|
||
| public virtual async Task<JsonResult> PackageDownloadsByVersionReport(string id, string[] groupby) |
There was a problem hiding this comment.
consider adding basic functional tests to check those endpoints
There was a problem hiding this comment.
Existing functional tests have been adapted to check these new endpoints.
chenriksson
suggested changes
May 11, 2017
|
|
||
| public virtual async Task<JsonResult> PackageDownloadsDetailReport(string id, string version, string[] groupby) | ||
| { | ||
| var report = await GetPackageDownloadsDetailReport(id, version, groupby); |
There was a problem hiding this comment.
Should the new methods also check if (_statisticsService == NullStatisticsService.Instance)?
|
|
||
| $('#statistics-retry').click(function () { | ||
| renderGraph(baseUrl, query); | ||
| }); |
There was a problem hiding this comment.
Does it need to register a new click callback on every failure?
There was a problem hiding this comment.
Yes, query can change.
chenriksson
approved these changes
May 11, 2017
xavierdecoster
approved these changes
May 12, 2017
loic-sharma
reviewed
May 12, 2017
| @@ -158,6 +158,63 @@ public StatisticsController(IStatisticsService statisticsService, IAggregateStat | |||
| return new HttpStatusCodeResult(HttpStatusCode.NotFound); | |||
| } | |||
|
|
|||
| var report = await GetPackageDownloadsByVersionReport(id, groupby); | |||
|
|
|||
| StatisticsPackagesViewModel model = new StatisticsPackagesViewModel(); | |||
There was a problem hiding this comment.
was existing code, but i'll fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an accessibility fix that has developed into a user experience improvement in general.
Previously, the PackageDownloadsDetail and PackageDownloadsByVersion pages would refresh every time the selected dimensions were changed. This was obviously bad for accessibility, as the focus would be lost every time the page refreshed, which disorients users.
I have fixed this.
Previously, the logic to inflate the table of information used by the browser was done in CSHTML, which is created server side. This logic is now done in JS using knockout (see
_PivotTable.cshtml). We already had knockout in our project so I used the existing version.Added a route that returns the statistics report in JSON.
Clicking upon a checkbox now makes an AJAX call that, on success, reinflates the information table using the statistics report in JSON. On error, it shows a message that asks the user if they would like to try again, which can be clicked to retry the AJAX call.
Tested on all the major browsers (
Firefox,Chrome,Edge).