-
Notifications
You must be signed in to change notification settings - Fork 40
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
Update line-chart interactions #2276
Conversation
@apburnes This is excellent! A huge improvement on what we had before. I just have a couple thoughts:
|
Codecov Report
@@ Coverage Diff @@
## develop #2276 +/- ##
===========================================
- Coverage 74.37% 74.35% -0.03%
===========================================
Files 111 111
Lines 6588 6610 +22
Branches 586 589 +3
===========================================
+ Hits 4900 4915 +15
- Misses 1688 1695 +7
Continue to review full report at Codecov.
|
Per our discussion in design sync and a followup convo, we intend to add these line charts back onto the data landing page, as well as incorporate onto the advanced data pages. Before finalizing/merging this PR:
Mock-ups for the advanced data pages can be found here: #2199 |
@apburnes Here's the updated mock-up for the line charts on the Raising and spending pages:
|
Updates for line charts on advanced data pages and data landing below. Advanced data pages
|
- See #2276 (comment) for checklist
@apburnes Based on our conversation, these are the following changes: On advanced data pages: On data landing page:
|
@JonellaCulmer Do you think we should change the date format on the data landing page for the chart to be the same as on the raising and spending data pages? Right now data landing is formatted |
@patphongs Yes, I agree. Thanks for catching. Do you think we need a testing ticket to verify the figures one more time before merge? |
@JonellaCulmer Yes, it would be good to validate the numbers one more time just to be sure. I've made a testing ticket here and assigned it to @PaulClark2. |
@apburnes Can you rebase from develop? It looks like there's a conflict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks really good. Approved! Thanks for all your efforts on this @apburnes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works/looks great at all sizes!
It is also useful to now have .content__section--ruled-responsive
without min-width for accordions inside tablists.
Does this need to be merged to dev for @PaulClark2 to test or should it remain un-merged until after testing? |
Summary (required)
Fixes interaction and scale placement of line chart.
Impacted areas of the application
This effects the
line-chart.js
and/data
landing page.