-
Notifications
You must be signed in to change notification settings - Fork 3
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
Narrative pages with animated transitions #288
Conversation
Pull Request Test Coverage Report for Build 478270227
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The transitions seem pretty smooth! It's great that the navigation for the sections is keyboard accessible. I just had a couple small questions.
// look like normal links. calling the setter is the critical step | ||
to={`${urlBase}/${activeSection - 1}`} | ||
onClick={() => { | ||
setActiveSection(activeSection - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to add e.preventDefault()
here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no actually I should have removed it in both places! and the preventdefault was what was causing the URL history to mysteriously not update in reference to your second comment, which I spent like ....... an hour-plus trying to debug. wow
|
||
// Jest/JSDOM don't support layout features | ||
// so we can't really test anything related to scroll position :( | ||
// (this also includes URL management because of how the page works) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean you wouldn't be able to look at the history
returned from renderNavigableApp
to assert the page URL?
Description of the change
Builds out the bulk (but not all) of the basic layout UI for the narrative pages.
Functionality you should see:
The animations are somewhat inspired by this article, although I didn't really use any of their code directly. We will probably wind up implementing a similar "wipe" transition on this site eventually, but full motion designs are still TBD. I'm also using a different animation library than they did, React Spring, which has a nice, concise API and seems to be the newest hotness? Seems good so far in this limited context. (And, after a brief scare, has been verified to work in IE 11!)
There is some slightly funky state management within this page to integrate the animations and prevent the router from clobbering the page (the section "routes" are kind of fake since they do not actually map to different components), but I tried to comment voluminously to keep it sensible.
Other work to do on this page (other than the charts, obviously) includes adding a page footer for navigating to the other narratives and adding some more interactions to the section navigation component (jumping to a specific section in addition to advancing one at a time). This PR was already getting big enough, though, so I will defer that stuff for the next one.
Also in this PR I removed all of the "data portal" stuff since that is now out of scope for the first release.
Type of change
Related issues
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: