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

Direct links to narrative sections #418

Merged
merged 18 commits into from
May 5, 2021

Conversation

macfarlandian
Copy link
Collaborator

@macfarlandian macfarlandian commented May 3, 2021

Description of the change

Revives and revamps the direct section linking feature (disabled here to unblock the v2 launch).

(The new behavior is different enough from the old that it's probably not a useful reference, but I include it nonetheless for context).

In some ways I think this is less complicated than it was the first time around because it relies on native browser behavior rather than JS for smooth scrolling and snapping. But it's not not complicated ... the new complexity here involves keeping track of where the user is on the page to prevent the linked section from being pushed down the viewport by content loading above it.

The end result should be that you can link directly to any section and it will scroll cleanly into your viewport when the page loads, and the page URL will stay in sync as you move up and down the page and can be grabbed at any time from the location bar to reflect your current view.

To summarize the highlights of how we got there:

  • On direct visits to a URL specifying a section > 1, the desired section is scrolled into view and any sections above it have their heights clamped to the height of the viewport; this prevents them from pushing the linked section out of position. When these sections scroll into view (i.e. if the user moves up the page), they expand to their natural height as needed. This expansion is animated to prevent any jarring "jumps" up the page; it's most noticeable on mobile where the sections are several screens tall.
  • On direct visits to the top of the page (no section or section 1), none of the above happens.
  • We detect when the top or bottom of an adjacent section comes into view and update the page URL to match. This responds to both scrolling and navigation via the left side buttons. (We were already doing this but I have improved the sensitivity somewhat.)
  • Sections will snap to the top of page when they are nearby, using the native Scroll Snap API, which was new to me but works pretty well, though a little differently from browser to browser. It's supported everywhere but IE 11 so is a good candidate for progressive enhancement, since it's not critical. This lets us drop the previous JS scrolling-and-snapping solution entirely, which is a net gain because that code pretty much didn't work at all in any browser 😎. The caveat here is that it gets confused by the apparent "scrolling" that occurs when the aforementioned clamped sections expand, and so needs to be disabled until we're done expanding all the sections. Again ... progressive enhancement 🎉 .

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

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 May 3, 2021

Pull Request Test Coverage Report for Build 811011867

  • 72 of 87 (82.76%) changed or added relevant lines in 12 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 73.411%

Changes Missing Coverage Covered Lines Changed/Added Lines %
spotlight-client/src/NarrativeLayout/NarrativeNavigation/AdvanceLink.tsx 1 2 50.0%
spotlight-client/src/NarrativeLayout/NarrativeNavigation/SectionLinks.tsx 2 3 66.67%
spotlight-client/src/index.tsx 0 1 0.0%
spotlight-client/src/routerUtils/normalizeRouteParams.ts 3 6 50.0%
spotlight-client/src/NarrativeLayout/useInternalNavigation.ts 32 41 78.05%
Files with Coverage Reduction New Missed Lines %
spotlight-client/src/index.tsx 1 0%
Totals Coverage Status
Change from base Build 790540479: -0.07%
Covered Lines: 2837
Relevant Lines: 3733

💛 - Coveralls

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.

This looks awesome...I noticed a behavior that I'm not sure is intended:

  1. Navigate to a section by entering it into the url (i.e. prison/3)
  2. Select a different Data Narrative (i.e. probation)
  3. See that it navigates to probation/3 - I would have expected it to go to probation/1

scrollToSection,
sectionsContainerRef,
getOnSectionExpanded,
onInViewChange,
Copy link

Choose a reason for hiding this comment

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

Wondering what you think about calling this variable onSectionInViewChange, might be easier to read later on without needing to checkout the note in the hook.

@macfarlandian
Copy link
Collaborator Author

@daschi hmmm good catch, that is indeed not intended. i'll look into it!

@macfarlandian
Copy link
Collaborator Author

turns out the same layout component was re-rendering when the narrative changed so the "initial" section of the first load was persisting as you navigated. (this wasn't happening until I consolidated everything into a hook instead of passing some of it as props, which was the last thing I did before opening this PR, so my earlier testing didn't catch it.) I added a more targeted reaction to clear it when you go to a new page, using a local Mobx observable

@macfarlandian macfarlandian merged commit 85c4003 into main May 5, 2021
@macfarlandian macfarlandian deleted the ian/395-routing-no-scroljacking branch May 5, 2021 19:11
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.

Allow direct links to narrative sections
3 participants