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

Categorical breakdown sections #332

Merged
merged 18 commits into from
Feb 3, 2021
Merged

Categorical breakdown sections #332

merged 18 commits into from
Feb 3, 2021

Conversation

macfarlandian
Copy link
Collaborator

Description of the change

Another visualization type ported over from v1. This is used in the prison releases, prison admissions, and supervision revocations sections.

Screen Shot 2021-01-29 at 5 31 14 PM

Screen Shot 2021-01-29 at 5 31 26 PM

This PR looks large but a pretty big chunk of it is whitespace changes and code copied wholesale from v1 so hopefully it is not too arduous to review. (I didn't break out the chart component as a separate PR because last time I did that, I found that not being able to actually, you know, see the chart meant I wound up having to change a bunch of stuff once I plugged it into the page.)

Highlights:

  • brings the BubbleChart component over into v2 (it now measures its own width instead of requiring its parent to provide it; unchanged otherwise)
  • uses existing ProportionalBar component
  • prevents janky initial animation for both charts by not rendering them when their width is zero (which it is when the component first mounts). This required a new mock to provide nonzero width values in the test environment.
  • animates the filter transition from chart to chart
  • most of the data handling was already done by the model (a refreshing contrast to what it looked like in the old component), only some light transformation of field names was needed here. The category identifiers are now display-ready (the old values were not internally meaningful so there wasn't any reason to preserve them), which led to some churn in the data fetch snapshot
  • there is now a reason for Metrics to carry their id as a property (another visualization type will use this same metric) so I added that to the constructor

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #301, closes #302

Checklists

Development

These boxes should be checked by the submitter prior to merging:

  • Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

  • This pull request has a descriptive title and information useful to a reviewer
  • This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
  • Potential security implications or infrastructural changes have been considered, if relevant

@coveralls
Copy link

coveralls commented Jan 30, 2021

Pull Request Test Coverage Report for Build 528977391

  • 82 of 83 (98.8%) changed or added relevant lines in 9 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.9%) to 67.416%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotlight-client/src/VizDemographicsByCategory/VizDemographicsByCategory.tsx 19 20 95.0%
Totals Coverage Status
Change from base Build 528931373: 0.9%
Covered Lines: 1874
Relevant Lines: 2690

💛 - Coveralls

Base automatically changed from ian/296-population-viz to master February 1, 2021 21:17
Copy link

@daschi daschi left a comment

Choose a reason for hiding this comment

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

The bubbles look really great! There's a slight delay between the labels and bubbles when you resize the window, but I'm assuming that is somewhat expected?

// because the bottom edge of each circle is aligned with the bottom,
// and the hypotenuse is a line connecting the centers, we know that
// c is the sum of the radii and b is the difference between them.
// Then we just solve for a
Copy link

Choose a reason for hiding this comment

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

very interesting, thanks for the explanation

) {
super(props);

makeObservable(this, { dataSeries: computed });
Copy link

Choose a reason for hiding this comment

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

It's pretty nice that the data can just be a computed value...I'm thinking we probably wouldn't be able to do something similar in pulse-dashboard because of the authentication..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i mean as long as the authentication data is also observable, there ought to be a way ...

@macfarlandian
Copy link
Collaborator Author

yeah that label issue is just a Semiotic thing, I wasn't able to figure out a way to change it the first time around (and ... I did not try this time)

@macfarlandian macfarlandian merged commit ff3fa3e into master Feb 3, 2021
@macfarlandian macfarlandian deleted the ian/301-bubble branch February 3, 2021 17:21
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.

Demographics by category viz Bubble + bar charts breakdown viz
3 participants