Implement Wayfinder 2.1 #565
Conversation
Pull Request Test Coverage Report for Build 2346506247Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
|
||
| return ( | ||
| <Container> | ||
| <Bar scrolled={percent || scrolledPercent} /> |
There was a problem hiding this comment.
When do we use scrolledPercent instead of percent?
There was a problem hiding this comment.
Personally I like the smoothly-updating progress bar with scrolledPercent rather than the percent we're passing in that's active section / total sections.
| shouldCloseOnOverlayClick={false} | ||
| > | ||
| <ModalHeading> | ||
| Introducing <strong>Wayfinder 2.1</strong> |
There was a problem hiding this comment.
So we want to tell our users "Introducing Wayfinder 2.1"? What is Wayfinder, should users know about this?
There was a problem hiding this comment.
It's more like a placeholder. Final copy is up to @hobuobi
…into nasaownsky/523-new-wayfinder
…into nasaownsky/523-new-wayfinder
There was a problem hiding this comment.
Apologies for the late review, but this is generally looks great; thank you so much for the detailed work! A few comments:
- Would make the entire bar clickable to open and close the Table of Contents, not just the pill to the left.
- After clicking a section in the Table of Contents, the bar should close.
- Arrow-blinking should only happen for the first section of the page, and should start 3 seconds after landing there.
- For now, we can get rid of the introduction modal ("Introducing Wayfinder 2.2").
Now that we aren't automatically adjusting the position of the page, the user can sometimes find themselves in a large whitespace between sections. We should consider reducing the amount of space between sections to minimize this effect.(EDIT: You can ignore this one; looking at some of the live Spotlight pages, I think the visualizations take up enough space to avoid this problem.)- The Racial Disparities is completely broken for me on all states. This might just be due to a data issue that I'm debugging on my side though, so if it's still working for others, it's probably just because of that.
|
@hobuobi a few comments about requested changes:
|
What if you try adding in event.stopPropagation() when handling the next/prev button events? Then that should prevent the bar from being triggered. |
Thanks, it worked! Also, after adding Maine there's some strange test errors appeared. Could you take a look? |
|
@nasaownsky are you talking about test errors that you're getting locally? It looks like all tests passed in your PR. |
|
@terryttsai Hm, strange. I think it's good to go then. |
|
What's your thoughts, @hobuobi? |
There was a problem hiding this comment.
All of this looks good to me!! Thank you so much @nasaownsky :)
Description of the change
Added new Wayfinder as requested in Issue, and removed scrolljacking.
Type of change
Related issues
Closes #515
Closes #523
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: