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

Daily auths report dashboard (LG-4648) #5

Merged
merged 14 commits into from
Aug 24, 2021

Conversation

zachmargolis
Copy link
Contributor

Features

Shows the last week of auths as a chart and also as a table

Shows all IAL 1 or all IAL2 at once

Filters that can be changed:

  • IAL level
  • Agency
  • Date picker (start/end dates)

Limitations

Currently the data is only served locally from data while

Screenshot

Screen Shot 2021-08-23 at 3 57 37 PM

package-lock.json Outdated Show resolved Hide resolved
src/daily-auths-report.js Outdated Show resolved Hide resolved
.flatMap(([issuer, ials]) => {
return Array.from(ials).map(([ial, data]) => {
const dayCounts = days.map(
(date) => data.filter((d) => d.date.valueOf() == date.valueOf())?.[0]?.count ?? 0
Copy link
Member

Choose a reason for hiding this comment

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

We don't have any ESLint rules currently other than Prettier, but if we did, I'd normally expect to see eqeqeq.

Do we want to build out the ESLint configuration? Or at least extend eslint:recommended? One though is we could wrap up the IDP's configuration as one of our workspace packages and publish it to use here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added eqeqeq and eslint:recommended in bb2ea6f

Sharing with the IDP would be nice, but would we have to clone the entire repo?

Copy link
Member

Choose a reason for hiding this comment

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

Sharing with the IDP would be nice, but would we have to clone the entire repo?

What I had in mind was to just npm publish from the workspace package directory.

It would be nice if we could point to a subfolder in a git repository as a dependency, but it seems like NPM doesn't support this. The next best thing might be git submodule + local path dependency, but... submodules. 😅

*/
function loadData(start, finish, fetch = window.fetch) {
return Promise.all(
utcDays(start, finish, 1).map((date) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very familiar with this library, but is it meant to return an array of unique days in the timeframe? Trying to set it to a singular day (start and end the same) returns an empty array for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

utcDays is a shorthand for the days interval function range:

https://github.com/d3/d3-time#interval_range

Returns an array of dates representing every interval boundary after or equal to start (inclusive) and before stop (exclusive).

So it's essentially [start, end) by day, so if start and end are the same I'd expect an empty array

Copy link
Member

Choose a reason for hiding this comment

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

Hm, okay. From a UX perspective I might expect it to behave as inclusive to the date range I select (i.e. if I want one day, pick that day as start and end).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm... I think that's a little bit a matter of expectations... if I set the UI to 8/14-8/21 (a week), it shows 8/14, 8/15, 8/17, 8/18, 8/19, 8/20** (7 days) which makes some sense. It's easy to update the UI to extend by another day if we wanted to see 8/21 included, so let's stick for now? It's basically saying midnight 8/14 to midnight 8/21

**after a fix to use UTC correctly in 66a8827

Copy link
Member

Choose a reason for hiding this comment

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

so let's stick for now?

Sounds reasonable 👍

Comment on lines +97 to +98
<input type="submit" value="Update" />
<a href="?"> (Reset) </a>
Copy link
Member

Choose a reason for hiding this comment

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

Would we care to avoid the full-page refresh? If so, I might expect we handle the form's onSubmit to set the new values into context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted the URL query params to stay in sync, and I didn't see a way for wouter to manipulate URL params, do you think it's worth switching libraries?

Copy link
Member

Choose a reason for hiding this comment

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

I wanted the URL query params to stay in sync, and I didn't see a way for wouter to manipulate URL params, do you think it's worth switching libraries?

Yeah, interesting... I see it's on the radar, but not very well supported. I don't suppose there's a path-based equivalent we could use for the sort of filtering we're doing here? Looks like there might be some workarounds in the above issue, though could be worth considering another library if it's too much trouble. I don't have much experience with it, but the "official" preact-router mentions querystrings?

Not much else here for options: https://github.com/preactjs/awesome-preact#routing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the time being, things load quickly and the browser is caching the data responses (at least locally).... will see how this goes when deployed in federalist + data served by cloudfront, and can re-eval then

src/report-filter-controls.js Outdated Show resolved Hide resolved
zachmargolis and others added 8 commits August 24, 2021 07:58
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
Co-authored-by: Andrew Duthie <aduth@users.noreply.github.com>
@zachmargolis zachmargolis merged commit fc460a3 into main Aug 24, 2021
@zachmargolis zachmargolis deleted the margolis-daily-auths-report branch September 15, 2021 17: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

2 participants