Revamp Spotlight state landing page #477

nasaownsky · 2021-08-19T12:17:13Z

Description of the change

Revamp Spotlight state landing page

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 #476

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

jessex

Holding off on a more thorough review for now, since there's not a whole lot to comment on just yet!

nasaownsky · 2021-08-24T17:33:40Z

@jessex Hey, Josh! So I have something like this for now:

@cawarren suggested using other charts to preview sentencing and parole data and they are much easier to manage.

The solution for the preview is using a boolean flag, depending on which we render different charts. The only thing I need to do is to get rid of prison population minimap. Maybe @macfarlandian know simple and effective solution for that?

macfarlandian · 2021-08-24T17:42:41Z

@nasaownsky hmm tricky .... I don't think that can be disabled, probably you will have to sub out the MinimapXYFrame component for a plain XYFrame in preview mode. It should take all of the same props with the exception of minimap so hopefully that won't be too bad

hobuobi · 2021-08-25T20:51:07Z

One quick thing: is it possible for this to be deployed to the staging site (spotlight-staging.recidiviz.org)? This would make it easy for @agronomic and I to review!

jessex

Looking good so far, @nasaownsky! Local testing went smoothly.

jessex · 2021-08-25T21:21:45Z

spotlight-client/src/OtherNarrativeLinks/OtherNarrativeLinks.tsx

@@ -91,6 +102,41 @@ const NarrativeLink: React.FC<{
    from: { opacity: 0 },
  }));

+  const renderChartPreview = () => {
+    if (tenantId === "US_ND") {


I wonder if there's a better approach for scaling this over time than just having to keep appending to this conditional block with new state codes. For example, would it be possible to configure which charts correspond to which narrative per state in /src/contentApi/sources/? Then all that renderChartPreview would need to do is invoke retrieveContent(tenantId) and then return the appropriate MetricVizMapper depending on the current narrative.id.

That would seem preferable because /src/contentApi/sources/ is already the location where we expect to add more content and config with each new state, so adding one more thing there means we won't miss it. But if we add one more thing that has to be updated with each new state here in this part of the codebase, we will inevitably miss it.

Thinking about implementation... If we'll have more states, then we still would need this conditional statement, telling us which exactly types of charts needed for a given state. Or you mean explicitly specify in api files (us_nd, us_pa) some sort of flag or list, that would tell us which charts to use as preview? This will need more exploration in types system. @jessex @macfarlandian any thoughts?

Also, other requested changes are resolved.

Yes I think expanding the us_nd and us_pa content objects to include this configuration sounds like a good idea - that has generally been the pattern we try to follow in this application. That configuration might look similar to what sections looks like for a narrative: an array of objects that specify a MetricTypeId and whatever other metadata you might need, if any.

It's fine to change the type definition for those content objects as needed, there aren't really any restrictions on what we can include there. It's just meant to collect as much tenant-specific configuration as possible in one place for visibility and ease of making changes.

spotlight-client/src/PageTenant/PageTenant.tsx

spotlight-client/src/VizDemographicsByCategory/VizDemographicsByCategory.tsx

spotlight-client/src/VizHistoricalPopulationBreakdown/VizHistoricalPopulationBreakdown.tsx

jessex · 2021-08-25T21:39:53Z

@macfarlandian I would deploy the frontend to staging myself but it looks like the .ZIP file with the env vars in 1Password is empty for spotlight-client. Would you 1) be able to update that and make sure it's complete and accurate so I can do these deploys myself going forward, and 2) deploy from this branch to staging so that @hobuobi can test? Thanks!

macfarlandian · 2021-08-25T21:54:37Z

@jessex uh oh i'll get on that

macfarlandian · 2021-08-25T22:01:49Z

@hobuobi latest version here is on staging now

coveralls · 2021-08-27T15:17:31Z

Pull Request Test Coverage Report for Build 1217031816

46 of 58 (79.31%) changed or added relevant lines in 9 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.1%) to 85.089%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spotlight-client/src/VizDemographicsByCategory/VizDemographicsByCategory.tsx	3	4	75.0%
spotlight-client/src/OtherNarrativeLinksPreview/OtherNarrativeLinksPreview.tsx	22	27	81.48%
spotlight-client/src/charts/WindowedTimeSeries.tsx	4	10	40.0%

Files with Coverage Reduction	New Missed Lines	%
spotlight-client/src/OtherNarrativeLinks/OtherNarrativeLinks.tsx	1	75.0%

Totals
Change from base Build 1134040090:	0.1%
Covered Lines:	1976
Relevant Lines:	2201

💛 - Coveralls

nasaownsky · 2021-08-30T15:36:17Z

@jessex Hey! In case you have missed this - take a look, it should be ready to go!

jessex

This is looking good to me! Thanks so much for this, @nasaownsky. Functionality looks solid in local testing, UI looks like the mocks, and I have no more code comments.

@macfarlandian, would you mind taking a pass through this this week? Ilya has shifted back to some final pre-launch updates for the Cost Calculator for the rest of this week so you have some time. But since this has some non-trivial modeling updates, I want to make sure you're on board before we merge.

spotlight-client/src/App.test.tsx

macfarlandian · 2021-09-01T00:40:40Z

spotlight-client/src/OtherNarrativeLinks/OtherNarrativeLinks.tsx

@@ -132,7 +160,6 @@ const OtherNarrativeLinks = (): React.ReactElement | null => {

  const narrativesToDisplay = [
    ...Object.values(tenant.systemNarratives),
-    tenant.racialDisparitiesNarrative,


bit of a problem here ... this component is also used at the bottom of each narrative page to provide navigation links to the other pages. The changes here break that feature, such that the link to Racial Disparities never shows up and all of these charts are rendered at the bottom of every page. I think this either needs to be broken out into a separate component (leaving the old component in place for uses other than the homepage) or it needs to have some kind of flag that puts it into "homepage mode" and causes it to render this new format with the chart previews and different link text. I think it would probably be more straightforward to just make a new component for the homepage and restore this one to its former state? It ought to be able to reuse all of the same styled components so hopefully that wouldn't lead to a lot of duplication.

spotlight-client/src/PageTenant/PageTenant.tsx

spotlight-client/src/RacialDisparitiesNarrativePage/RacialDisparitiesNarrativePage.test.tsx

macfarlandian · 2021-09-01T00:58:17Z

spotlight-client/src/contentApi/types.ts

@@ -151,6 +151,8 @@ type SystemNarrativeSection = NarrativeSection & {

 export type SystemNarrativeContent = {
  title: string;
+  subtitle?: string;
+  preview?: MetricTypeId;


I don't think the preview field is actually optional in practice? could simplify things a little by just having it be required at the type level, we wouldn't have to check if it's defined before rendering

macfarlandian · 2021-09-01T01:00:03Z

spotlight-client/src/contentModels/types.ts

@@ -90,4 +89,4 @@ export type SystemNarrativeMapping = {
  [key in SystemNarrativeTypeId]?: SystemNarrative;
 };

-export type Narrative = SystemNarrative | RacialDisparitiesNarrative;


per comments about the links above, I don't think we are going to want to make this change in the end.

spotlight-client/src/charts/BubbleChart.tsx

macfarlandian · 2021-09-01T01:15:48Z

spotlight-client/src/charts/WindowedTimeSeries.tsx

+            axes={[
+              {
+                orient: "left",
+                tickFormat: formatAsNumber,
+                tickSize: 0,
+              },
+            ]}
+            baseMarkProps={BASE_MARK_PROPS}
+            lines={data}
+            lineStyle={(d: DataSeries) => ({
+              fill:
+                highlighted && highlighted.label !== d.label
+                  ? highlightFade(d.color)
+                  : d.color,
+              stroke: colors.background,
+              strokeWidth: 1,
+            })}
+            // @ts-expect-error Semiotic typedefs are incomplete, this works
+            lineType={{ type: "stackedarea", sort: null }}
+            // @ts-expect-error Semiotic typedefs are wrong, can be true for default matte
+            matte
+            pointStyle={{ display: "none" }}
+            xAccessor="date"
+            yAccessor="count"


I think by doing it this way we are creating a bunch of expensive React and Semiotic stuff that we don't intend to render, as well as defining most of the props redundantly?

Rather than duplicating all these props, we should just collect them in a base object that we extend as needed with the additional props needed for the minimap. And instead of instantiating both Frame components we should just conditionally pick one ... could either store that as a variable or conditionally render {preview ? <XYFrame {...chartProps} /> : <MinimapXYFrame {...chartProps} />} instead of what's currently there.

spotlight-client/src/charts/WindowedTimeSeries.tsx

macfarlandian · 2021-09-02T18:59:15Z

realized I forgot to post one other thing ... I noticed that the tooltips can be cut off in the "preview" charts if they are close to the right edge. example screenshots:

nasaownsky · 2021-09-08T15:19:47Z

Hey, @macfarlandian! So I've made all the changes you have requested. Check that out!

macfarlandian

getting very close, @nasaownsky ! other than a couple of nits I commented on here, the main remaining issue is with the navigation links at the bottom of the narrative pages.

If you were to look at, e.g., this page on the live site, it still looks a little different than it does with these changes:

The layout is a bit tighter (can fit four to a row) and the labels don't say "Data" like they do on the new homepage design. We still want the links on those pages to remain unchanged from what they were before, so I think there needs to be even more separation between the components than you have here. And just to give you a heads-up, there is a further change coming (still being spec'ed out, will be a separate ticket) that will include the Racial Disparities link on the homepage but also reshuffle the order, so the functionality of the homepage versus the other pages will diverge even more. I don't think there's going to be much benefit in trying to adapt one component to render both versions, so if breaking the homepage links out into a whole new component makes this easier then you should feel free to make that change now

spotlight-client/src/charts/WindowedTimeSeries.tsx

spotlight-client/src/SystemNarrativePage/constants.ts

nasaownsky · 2021-09-08T18:54:33Z

@macfarlandian ok, I've added brand new component OtherNarrativeLinksPreview and since the Racial Disparities not included on the homepage, there is no need to modify types and extract NarrativeLink from both OtherNarrativeLinksPreview and OtherNarrativeLinks as its rely on different types and there is no specification for Racial Disparities for now.

macfarlandian

did one more QA pass and noticed that some of the preview labels didn't match the data we were showing ... noted them in the comments here. I think that should be the last thing on this PR

spotlight-client/src/contentApi/sources/us_nd.ts

spotlight-client/src/contentApi/sources/us_pa.ts

nasaownsky · 2021-09-09T11:06:24Z

Done!

macfarlandian

🚀

Added subtitles, state landing revamping

f13ff4f

nasaownsky requested review from jessex and macfarlandian August 19, 2021 12:17

jessex reviewed Aug 19, 2021

View reviewed changes

Added charts preview to the tenant page

6cbc429

nasaownsky added 2 commits August 25, 2021 19:40

Charts preview improvements

5d12516

Updated copy

a5ae4a0

jessex requested changes Aug 25, 2021

View reviewed changes

nasaownsky added 5 commits August 26, 2021 19:04

Lint fixes

995581b

Extracted core visualization for Viz elements

72620c3

Updated renderChartPreview logic

c3022da

Fixed tests

1ed5b4c

Removed logs

422681a

nasaownsky marked this pull request as ready for review August 27, 2021 15:19

nasaownsky added 3 commits August 30, 2021 18:48

Removed Racial Disparities link

19e31d7

Tests fixes

fce98f5

Added test case

cd49440

jessex approved these changes Aug 31, 2021

View reviewed changes

macfarlandian requested changes Sep 1, 2021

View reviewed changes

Fixes

c05dd43

macfarlandian reviewed Sep 8, 2021

View reviewed changes

spotlight-client/src/charts/WindowedTimeSeries.tsx Outdated Show resolved Hide resolved

spotlight-client/src/SystemNarrativePage/constants.ts Outdated Show resolved Hide resolved

More fixes

37010b8

macfarlandian reviewed Sep 8, 2021

View reviewed changes

Copy fixes

7c86231

macfarlandian approved these changes Sep 9, 2021

View reviewed changes

jessex merged commit 04286a9 into main Sep 13, 2021

jessex deleted the nasaownsky/state-landing-page branch September 13, 2021 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp Spotlight state landing page #477

Revamp Spotlight state landing page #477

nasaownsky commented Aug 19, 2021 •

edited by jessex

jessex left a comment

nasaownsky commented Aug 24, 2021

macfarlandian commented Aug 24, 2021

hobuobi commented Aug 25, 2021

jessex left a comment

jessex Aug 25, 2021

nasaownsky Aug 26, 2021

macfarlandian Aug 26, 2021

jessex commented Aug 25, 2021

macfarlandian commented Aug 25, 2021

macfarlandian commented Aug 25, 2021

coveralls commented Aug 27, 2021 •

edited

nasaownsky commented Aug 30, 2021

jessex left a comment

macfarlandian Sep 1, 2021

macfarlandian Sep 1, 2021

macfarlandian Sep 1, 2021

macfarlandian Sep 1, 2021

macfarlandian commented Sep 2, 2021

nasaownsky commented Sep 8, 2021

macfarlandian left a comment

nasaownsky commented Sep 8, 2021

macfarlandian left a comment

nasaownsky commented Sep 9, 2021

macfarlandian left a comment

Revamp Spotlight state landing page #477

Revamp Spotlight state landing page #477

Conversation

nasaownsky commented Aug 19, 2021 • edited by jessex

Description of the change

Type of change

Related issues

Checklists

Development

Code review

jessex left a comment

Choose a reason for hiding this comment

nasaownsky commented Aug 24, 2021

macfarlandian commented Aug 24, 2021

hobuobi commented Aug 25, 2021

jessex left a comment

Choose a reason for hiding this comment

jessex Aug 25, 2021

Choose a reason for hiding this comment

nasaownsky Aug 26, 2021

Choose a reason for hiding this comment

macfarlandian Aug 26, 2021

Choose a reason for hiding this comment

jessex commented Aug 25, 2021

macfarlandian commented Aug 25, 2021

macfarlandian commented Aug 25, 2021

coveralls commented Aug 27, 2021 • edited

Pull Request Test Coverage Report for Build 1217031816

💛 - Coveralls

nasaownsky commented Aug 30, 2021

jessex left a comment

Choose a reason for hiding this comment

macfarlandian Sep 1, 2021

Choose a reason for hiding this comment

macfarlandian Sep 1, 2021

Choose a reason for hiding this comment

macfarlandian Sep 1, 2021

Choose a reason for hiding this comment

macfarlandian Sep 1, 2021

Choose a reason for hiding this comment

macfarlandian commented Sep 2, 2021

nasaownsky commented Sep 8, 2021

macfarlandian left a comment

Choose a reason for hiding this comment

nasaownsky commented Sep 8, 2021

macfarlandian left a comment

Choose a reason for hiding this comment

nasaownsky commented Sep 9, 2021

macfarlandian left a comment

Choose a reason for hiding this comment

nasaownsky commented Aug 19, 2021 •

edited by jessex

coveralls commented Aug 27, 2021 •

edited