Page footers #291

macfarlandian · 2021-01-12T00:38:36Z

Description of the change

Adds a standard footer on every page, and an additional footer for narrative pages with links to the other narratives.

Some side effects to this otherwise straightforward task:

this required the section navigation UI to be sticky instead of fixed; to make that work in IE I had to include a polyfill and adapt the markup slightly to accommodate it. (Things had to be nested a little differently to account for some spacing collapsing when the "sticky" element takes on position: fixed in IE.)
state management around translating the URL to the corresponding Narrative instance was getting redundant, especially once the the footer starting needing to do it also. Instead I moved this into Mobx: the route parameters become observable, and then the corresponding Tenant and/or Narrative is a computed property based on that. withRouteSync now does this instead of explicitly calling setter methods or creating the Tenant instance directly.

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

Part of #276

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 · 2021-01-12T00:41:03Z

Pull Request Test Coverage Report for Build 481321558

54 of 59 (91.53%) changed or added relevant lines in 14 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.6%) to 64.653%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
spotlight-client/src/index.tsx	0	1	0.0%
spotlight-client/src/NarrativeFooter/NarrativeFooter.tsx	14	18	77.78%

Totals
Change from base Build 478300694:	0.6%
Covered Lines:	1407
Relevant Lines:	2126

💛 - Coveralls

jovergaag

Cool! It looks really beautiful.

I did find some of the transitions to be a little choppy, is there any way to smooth these out? Also found a small IE11 bug and mobile styling issue.

jovergaag · 2021-01-12T18:55:33Z

spotlight-client/src/SystemNarrativePage/Section.tsx

-        <SectionTitle>{section.title}</SectionTitle>
-        <SectionBody>{HTMLReactParser(section.body)}</SectionBody>
-      </SectionCopy>
+      <Sticker>


I'm finding the transition between sections to be pretty choppy as I scroll down the page.

hm yeah ... it varies a bit by browser in my experience but it especially does not like it if you try to scroll while the animation is still in progress. It's definitely still kinda buggy! I think though that I want to collect some more feedback and deal with this animation tuning in a separate PR (since I don't think it's directly affected by any of the changes in this one)

jovergaag · 2021-01-12T19:12:34Z

spotlight-client/src/NarrativeFooter/NarrativeFooter.tsx

+                  })}
+                  onMouseOver={transitionArrow({ index, visible: true })}
+                  onFocus={transitionArrow({ index, visible: true })}
+                  onMouseOut={transitionArrow({ index, visible: false })}


In IE11 I found that one of the footer link arrows remained visible after clicking through to a different narrative. Was able to reproduce with:

From the Sentencing page, scroll to the bottom of the page to see the Narrative Footer

Click on the Probation footer link

Scroll to the bottom of the page and see that the Prison arrow is still visible, even clicking elsewhere on the page.

oh good catch! I think I see the issue

jovergaag · 2021-01-12T19:14:22Z

spotlight-client/src/SystemNarrativePage/SystemNarrativePage.tsx

+      <NavContainer>
+        <Sticker>
+          <NavStickyContainer>
+            <SectionNavigation


I found that in IE11, clicking on the arrows in the SectionNavigation to click through to the next section to be quite slow. Enough to make me wonder the first time I clicked if something was broken. Have you noticed this?

Another question on the section navigation. I was a little frustrated when at the bottom of the page, i couldn't remember which narrative I was on previously so I clicked the 'Back' button, but had to hit back 8 times to actually get back to the previous Narrative. What is the reason to keep the section number in the URL? Are the sections going to be so large that the user will want to navigate between them rather than scrolling between them?

hm. it's definitely less snappy in IE 11 but for me it's not quite that bad. There is a delay of about a second or so though. I am not sure what our IE support level is going to be for this product but at the moment I am just aiming for "not completely broken" and by that standard I think this squeaks by?

Interesting point though about the URLs. I think I tend to agree with you, but this came down as a product requirement, to be able to link directly to a section. We may find that in practice we don't like the way it works. Most of them are not going to be particularly large; I think the majority will fit on a single screen.

I could try replacing the current history item when the section changes, rather than pushing it onto the stack? I don't know if that would just be confusing in a different way, though.

I did give the replacement approach a quick try and I feel like it's .... not obviously better? you wind up on the last section of the previous page if you click on one of those footer links and then hit the back button. It feels ... also weird. Anything more sophisticated than that, or a change in the URL scheme, is a product discussion I think. I will make a note of it for later review though

jovergaag · 2021-01-12T19:17:50Z

spotlight-client/src/SystemNarrativePage/SystemNarrativePage.tsx

-      />
+      <NavContainer>
+        <Sticker>
+          <NavStickyContainer>


I'm getting a console warning from the NavStickyContainer

I also think that the mobile styling needs to be adjusted now that this NavStickyContainer has been added.

oh yeah there is no mobile styling yet. designs should be landing sometime this week so I am punting it for now

also, I saw that warning too. React is starting to complain about some of the dependencies using deprecated APIs. I am not super inclined to care about this at this point? It only gets raised in Strict Mode, which in practice I am finding pretty annoying and have been thinking about disabling. Maybe it's helpful as a debugging tool but as a general development tool I think it's way too noisy about stuff that's not in our direct control. (I have not been able to find a direct replacement for this dependency, one that simply wraps the polyfill in React, and I don't really think it's worth trying to figure out how to reimplement it without findDOMNode. There are various other pure-JS or pure-React libraries, in fact I used one of them in Spotlight v1 but the performance of this is way better in every browser except IE 11 because it uses the native CSS solution instead of janky JavaScript.)

jovergaag

🌸

(the cherry blossom is the only emoji that autofills for ':spring' - so there you have it)

macfarlandian added 5 commits January 11, 2021 11:45

sticky position with IE 11 polyfill

990da20

move route param handling to the datastore

32e0dfc

add narratives footer

0c8130e

add site footer

b33662f

keep section param in sync

ab67d3e

macfarlandian requested review from ohaibbq, daschi and jovergaag January 12, 2021 00:38

jovergaag requested changes Jan 12, 2021

View reviewed changes

macfarlandian added 2 commits January 12, 2021 12:53

Fix arrow rendering bug

38a7109

disable React strict mode

f7d7a0a

jovergaag approved these changes Jan 12, 2021

View reviewed changes

macfarlandian merged commit 69d222f into master Jan 12, 2021

macfarlandian deleted the ian/276-footer branch January 12, 2021 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Page footers #291

Page footers #291

macfarlandian commented Jan 12, 2021

coveralls commented Jan 12, 2021 •

edited

Loading

jovergaag left a comment

jovergaag Jan 12, 2021

macfarlandian Jan 12, 2021

jovergaag Jan 12, 2021

macfarlandian Jan 12, 2021

jovergaag Jan 12, 2021 •

edited

Loading

jovergaag Jan 12, 2021 •

edited

Loading

macfarlandian Jan 12, 2021

macfarlandian Jan 12, 2021

jovergaag Jan 12, 2021

jovergaag Jan 12, 2021

macfarlandian Jan 12, 2021

macfarlandian Jan 12, 2021

jovergaag left a comment

Page footers #291

Page footers #291

Conversation

macfarlandian commented Jan 12, 2021

Description of the change

Type of change

Related issues

Checklists

Development

Code review

coveralls commented Jan 12, 2021 • edited Loading

Pull Request Test Coverage Report for Build 481321558

💛 - Coveralls

jovergaag left a 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

jovergaag Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

jovergaag Jan 12, 2021 • 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

jovergaag left a comment

Choose a reason for hiding this comment

coveralls commented Jan 12, 2021 •

edited

Loading

jovergaag Jan 12, 2021 •

edited

Loading

jovergaag Jan 12, 2021 •

edited

Loading