Add banner with 'About' modal; move legend to be underneath plot#39
Add banner with 'About' modal; move legend to be underneath plot#39david-mears-2 merged 14 commits intomainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #39 +/- ##
==========================================
+ Coverage 98.01% 98.08% +0.07%
==========================================
Files 19 20 +1
Lines 453 470 +17
Branches 108 121 +13
==========================================
+ Hits 444 461 +17
Misses 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
david-mears-2
requested review from
EmmaLRussell and
kgaythorpe
and removed request for
EmmaLRussell
EmmaLRussell
left a comment
EmmaLRussell
left a comment
There was a problem hiding this comment.
Banner looks good!. The layout change would look a bit neater if the legend was aligned with the left side of the plot I think. Also the "Legend" label on the left looks a bit odd - tbh I don't think it's really needed and would probably just get rid of it.
I think since the text of the About box specifies that the paper isn't published yet, it would be better to have the two references to the paper "Gaythorpe et al" just be plain text for now rather than links don't work - since you'll be changing it anyway when the paper is published.
We generally do links to other domains as target="_blank" to open new tabs - I'd be inclined to do that for the gavi and vaccineimpact.org links.
src/components/AppHeader.vue
Outdated
| id="aboutLink" | ||
| @click.prevent="aboutModalVisible = true" | ||
| href="#" | ||
| class="text-fg-brand underline" |
There was a problem hiding this comment.
Do you need class underline here when you've already put text-decoration: underline; in main.css?
There was a problem hiding this comment.
Indeed not, this is a relic - now banished to the past by defining button.link in main.css
src/components/AppHeader.vue
Outdated
| <div class="flex flex-col gap-4 max-w-100 items-end"> | ||
| <!-- TODO: When paper is published, add the href. --> | ||
| <p class="text-right">This visualization tool accompanies <a href="#">Gaythorpe et al.</a></p> | ||
| <a |
There was a problem hiding this comment.
Really this ought to be a button rather than a link for accessibility - but by definition it's not a very accessible app for people with reduced vision anyway, so up to you ...
https://www.a11y-collective.com/blog/button-vs-link/
There was a problem hiding this comment.
I see: because it doesn't actually navigate to a different page, it should be a button. This fix is easy so I'll do it.
It's supposed to be horizontally centered, but your screenshot shows it not doing that, which I don't understand... rather than dig into the weeds there I will simply left-align the color legend. |
Yeah, this isn't needed is it. I was intending to change that text to read either 'Location' or 'Disease' (as in WIP branch #33) so I didn't delete it. But maybe even then, it's still unnecessary. Removed! |
…ction from plotConfiguratoin util
david-mears-2
force-pushed
the
mrc-6841-header
branch
from
841a570 to
cc7813d
Compare
👍 |
👍 |
Add a warning about not using or sharing the data
|
NB Since review I've added a little note of the vaxviz version to the About modal, so that we get more visibility when deploying different versions. New version number is 0.1.0 and I'll use 1.x.x once we are officially public with the app. 
|
3 participants
Because adding the header took up vertical space, I ended up moving the legend (which can get rather long) to be a horizontal element, underneath the plot. Some of the now-empty bottom left space will be taken up when we add (focus-level) filters, and download buttons.
The big red warning sign that the estimates are PROVISIONAL will be in a different PR.
Header:
About modal:
(Currently the text contains caveats since the estimate data displayed is not ready)