Data models and routing for "system overview" narratives #285

macfarlandian · 2020-12-19T02:14:28Z

Description of the change

This creates the content and models for the "system overview" narratives, which are analogous to the main pages from Spotlight v1, and plugs them into the routing and navigation system.

Now that the scaffolding is all in place, this was pretty straightforward! Note that most of the content I had originally assigned to the Metrics and Collections is really more appropriate for these narratives (which did not really exist in the product requirements until about a week ago), so I mostly just moved that over. I tried to replace the metric titles with what was in the data portal mocks and sub in something sensible if there was nothing there, but none of that should be considered final at this point. (all of those "description" fields are now just placeholders because I don't have anything to put there, and I am frankly not even sure if we will wind up needing those fields at all; they don't appear in the latest iterations of the mocks, but we can worry about that later).

In the future we will have to disambiguate between different kinds of narratives, which is why the routing-related code refers to "narrativeTypeId" more generically; within the data models though, we will want to keep them separate to make UI treatment easier; different groups of narratives are not likely to resemble one another in structure or presentation.

Also note that I renamed "PageNarrativeHome" to "PageNarrativeList", which seems to have confused the Github diffing elves. Sorry! 🧝🏻‍♂️

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

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 · 2020-12-21T17:32:30Z

Pull Request Test Coverage Report for Build 439152675

45 of 47 (95.74%) changed or added relevant lines in 14 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 63.465%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spotlight-client/src/PageNarrative/PageNarrative.tsx	5	6	83.33%
spotlight-client/src/routerUtils/normalizeRouteParams.ts	6	7	85.71%

Totals
Change from base Build 431307609:	0.6%
Covered Lines:	1314
Relevant Lines:	2026

💛 - Coveralls

jovergaag

My big question here is around Narrative/Collection The models are quite different, but where and how they are used is so similar. Clicking 'Collections' in the navbar routes to narratives. As someone not knowing the future product well enough to know what the content of these pages will be, I have found it pretty confusing. I'm guessing that the Collections data will be interwoven into the Narratives copy? But why not call the route collections to match the nav?

What am I missing that explains why these don't line up?

jovergaag · 2020-12-22T20:33:19Z

spotlight-client/src/PageNarrative/PageNarrative.tsx

+};
+
+const PageNarrative: React.FC<PageNarrativeProps> = ({ narrativeTypeId }) => {
+  const tenant = useDataStore().tenantStore.currentTenant;


I imagine that this currentTenant is going to be used and accessed on many pages. Thoughts on adding these frequently accessed stores to the StoreProvider for easier access?

meaning, like, it would be useDataStore().currentTenant instead of this? yeah maybe that's a good idea ... I keep writing this line over and over again

Or useTenantStore().currentTenant

oh i see what you meant .... I think it is simpler for there to just be one context that contains the entire data store? that is sort of what they recommend but maybe it is not strictly necessary ... i haven't tried that

jovergaag · 2020-12-22T20:40:12Z

spotlight-client/src/contentModels/SystemNarrative.ts

+// along with this program.  If not, see <https://www.gnu.org/licenses/>.
+// =============================================================================
+
+import { SystemNarrativeContent } from "../contentApi/types";


I don't see a test for this class or createSystemNarrative. None of it very complicated logic so not critical, but it might be nice to test that the section is omitted if the metric doesn't exist.

that is tested as part of the createTenant tests, which integrate this module; the tenant has system narratives test verifies that the instantiated narratives correspond to what was in the content blob. I didn't really see any extra edge cases that seemed to warrant additional tests on this factory specifically

jovergaag · 2020-12-22T20:50:19Z

spotlight-client/src/routerUtils/getUrlForResource.ts

@@ -50,9 +54,12 @@ function getUrlForResource(opts: GetUrlOptions): string {
      return `/${makeRouteParam(
        opts.params.tenantId
      )}/${DataPortalSlug}/${makeRouteParam(opts.params.metricTypeId)}`;
-    case "narratives":
+    case "narrative list":


I keep getting caught up on the Collections -> Narratives mismatch. The Navbar is 'Collections' but the route and pages are Narrative (or some derivation of narrative). Why not make the route and pages 'Collection' derived?

yeah it is very confusing and silly. "collections" used to be the word for what might properly be called "topics" or "tags" in the context of the data portal. then at some point in the latest round of designs it also got adopted as the "display title" of the page that we still refer to internally as "narratives", but they haven't really settled on that as the final title (it might be "narratives", it might be "stories", it might be something else) ... so I was trying to avoid using it in too many places. Typing that out, though, I can see that it doesn't make that much sense! At the very least I should have the title and the URL match up (and if they change later then they both change). But I think the names of components and stuff should be more stable than the display values, to avoid pointless churn, unless they drift so far apart as to become an impediment, so I would leave those as "narrative". Words! 💀

More fun facts: I don't think there will actually be any relationship between the narratives and collections. They just so happen to completely overlap at the moment but that will not continue to be true. Also, I learned yesterday that the entire data portal feature is getting de-prioritized out of MVP scope, so the "explore" tab is going to come out of the navigation and get put on ice for a while. I may just remove the "collection" data model entirely so it doesn't turn into cruft, since it is not doing anything useful yet anyway.

macfarlandian · 2020-12-22T23:32:24Z

@jovergaag i think i would rather not mess with the data portal and "collections" stuff any further as part of this PR since it's not directly related to this functionality. your point is well taken though about what a general mess it is and I will aim to clean it up in the near future

jovergaag

Rock on 🎸

jovergaag · 2020-12-23T00:12:23Z

spotlight-client/src/PageMetric/PageMetric.tsx

@@ -25,7 +25,7 @@ import withRouteSync from "../withRouteSync";
 type PageMetricProps = RouteComponentProps & { metricTypeId?: MetricTypeId };

 const PageMetric: React.FC<PageMetricProps> = ({ metricTypeId }) => {
-  const tenant = useDataStore().tenantStore.currentTenant;
+  const { tenant } = useDataStore();


Oh ya thats nicer.

macfarlandian added 4 commits December 18, 2020 12:48

update ND content with system narratives

11ba6b1

create model for narratives

6ea16b2

add narratives to router and UI

9bf739a

resolve TODO

1a1b935

macfarlandian requested review from daschi and jovergaag December 19, 2020 02:14

jovergaag reviewed Dec 22, 2020

View reviewed changes

macfarlandian added 2 commits December 22, 2020 15:20

change narratives url

a4afdf0

observable shortcut to current tenant

776ca47

jovergaag approved these changes Dec 23, 2020

View reviewed changes

macfarlandian merged commit ca936a6 into master Dec 23, 2020

macfarlandian deleted the ian/275-narrative-model branch December 23, 2020 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data models and routing for "system overview" narratives #285

Data models and routing for "system overview" narratives #285

macfarlandian commented Dec 19, 2020 •

edited by jessex

Loading

coveralls commented Dec 21, 2020 •

edited

Loading

jovergaag left a comment •

edited

Loading

jovergaag Dec 22, 2020

macfarlandian Dec 22, 2020

jovergaag Dec 23, 2020

macfarlandian Dec 23, 2020

jovergaag Dec 22, 2020

macfarlandian Dec 22, 2020

jovergaag Dec 22, 2020

macfarlandian Dec 22, 2020

macfarlandian commented Dec 22, 2020

jovergaag left a comment

jovergaag Dec 23, 2020

Data models and routing for "system overview" narratives #285

Data models and routing for "system overview" narratives #285

Conversation

macfarlandian commented Dec 19, 2020 • edited by jessex Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

coveralls commented Dec 21, 2020 • edited Loading

Pull Request Test Coverage Report for Build 439152675

💛 - Coveralls

jovergaag left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarlandian commented Dec 22, 2020

jovergaag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarlandian commented Dec 19, 2020 •

edited by jessex

Loading

coveralls commented Dec 21, 2020 •

edited

Loading

jovergaag left a comment •

edited

Loading