display Fetch Job stats via icon click #114
Conversation
peterVG
changed the title
| <td>{{ mets_fetch_job.total_packages }}</td> | ||
| <td>{{ mets_fetch_job.total_packages }} packages | ||
| <i class="fa fa-info-circle fa-lg" id="{{ mets_fetch_job.id }}" style="margin-left: 5px;"></i><br/> | ||
| <div id="stats-{{ mets_fetch_job.id }}" class="stats" style="font-size:80%; color: #999;"> |
There was a problem hiding this comment.
I'd recommend using Bootstrap tags and classes instead of a custom style tag to manage the styles. So for instance, inside this <div> you could do something like
<span class="text-muted">
<small>
{{ mets_fetch_job.total_aips }} AIPs<br>
{{ mets_fetch_job.total_sips }} SIPs<br>
{{ mets_fetch_job.total_dips }} DIPs<br>
{{ mets_fetch_job.total_replicas }} replicated AIPs<br>
{{ mets_fetch_job.total_replicas }} deleted AIPs
</small>
</span>
We do that elsewhere in the code base as well, e.g.: https://github.com/artefactual-labs/AIPscan/blob/main/AIPscan/Reporter/templates/report_largest_files.html#L74. The main advantage this change would give is if we want to change what our text-muted style looks like, we can do so in one place and have it apply to the whole project.
Full transparency, this on my mind because I think the low contrast might be a bit of an accessibility issue, and so we might want to find a less-muted shade of grey. But that's a codebase-wide issue and might be better addressed in a separate PR.
There was a problem hiding this comment.
Yes, custom inline CSS is a bad temp hack. Thanks for this cleanup suggestion. Done.
| @@ -49,7 +49,16 @@ | |||
| <tr> | |||
| <td>{{ mets_fetch_job.download_start }}</td> | |||
| <td>{% if mets_fetch_job.download_end %} {{ mets_fetch_job.download_end - mets_fetch_job.download_start }} {% endif %}</td> | |||
| <td>{{ mets_fetch_job.total_packages }}</td> | |||
| <td>{{ mets_fetch_job.total_packages }} packages | |||
| <i class="fa fa-info-circle fa-lg" id="{{ mets_fetch_job.id }}" style="margin-left: 5px;"></i><br/> | |||
There was a problem hiding this comment.
For HTML5, the preferred line break tag is <br>, no trailing space/slash needed: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/br
There was a problem hiding this comment.
I learned basic HTML back in the 90's and this was considered very important (yet totally irrelevant in the end) practice.
| {{ mets_fetch_job.total_aips }} AIPs <br /> | ||
| {{ mets_fetch_job.total_sips }} SIPs <br /> | ||
| {{ mets_fetch_job.total_dips }} DIPs <br /> | ||
| {{ mets_fetch_job.total_replicas }} replicated AIPs <br /> |
There was a problem hiding this comment.
Nit: We often call these "replicated AIPs", but it's a bit ambiguous whether that means the original or the replica. I think it might be more accurate or at least unambiguous to call them "AIP replicas".
There was a problem hiding this comment.
Also, great nit-picking. I do this kind of semantic hair splitting all the time and appreciate it when others do it too. Accuracy is in our field is important. Starting with how we describe and display the features in our systems.
| $('.stats').hide(); | ||
| $('.fa-info-circle').click(function() { | ||
| if($('#stats-'+this.id).is(':visible')) | ||
| $('#stats-'+this.id).hide(); | ||
| else | ||
| $('#stats-'+this.id).show(); | ||
| }); |
There was a problem hiding this comment.
Succinct, readable, and effective - nice job!
One minor nit/linting issue is that it'd be great to have opening and closing curly braces aligned - these are fairly small closures so not an immediate practical need maybe, but having them properly aligned really helps with readability when code grows. In practical terms I think it would just mean moving the closing });s here two spaces to the left.
There was a problem hiding this comment.
This is the sort of thing that we will ideally eventually have a JS linter to help spot (and potentially even auto-correct for us, like black does for our Python). And it's absolutely a nit, so I'll leave it up to you to decide if it's worth the time.
There was a problem hiding this comment.
I'll take any suggestions on improving the readability of Javascript / JQuery, lol.
Have we shortlisted JS Linters yet? I'll gladly install one.
I was actually trying to be consitent with 2-space indents on my JS code but this just goes to show how impossible that is to maintain manually without a Linter. I fixed the indent issue you found. There are probably more :-)
Making use of the Fetch Job statistics that are already being stored in the database, this commit displays them on the Storage Service template. Rather than clutter the screen with stats for multiple Fetch Jobs, it only reveals the stats when clicking an Info icon. Clicking the icon again re-hides the stats info.
Connected to #47