Vulcan: Add TOC (Desktop/ Mobile) and Scroll Indicator #1923
Conversation
This is a simple component that shows the scroll percentage of the page. It takes an optional `scrollContainerRef` prop that will limit the indicator to the scrollable area of the container. If not provided, it will default to the height of the window. When using the ref, we will also stay at 0 until we have scrolled past the top of the container. This is useful since we are rendering this below the header, and we don't want to show the scroll percentage of the header. Only the scroll percentage of the content.
I was messing around and wasn't sure exactly the behavior we might want. So I added some options to the scroll indicator. We can choose to always show the indicator (on page load it takes up space and looks odd) or we can choose to only show it when the user scrolls. Similarly, we can choose to show the indicator after the user moves past the content or we can hide it after the user moves past the content.
Just a syntax mistake
This does not handling of rendering, but instead just handles the main logic of the TOC. You push on a ref object and it's title in the TOC. The context will automatically watch the scrolling and update to the item whose ref is currently in view. Currently in view is defined as the ref is in the view port and the largest portion of the ref is in the view port. This means a tiny 50px div in the center of the viewport will be considered in view, but a 1000px div that is 1px off the top of the viewport will not be considered in view. This is a good compromise between the two, but may need to be tweaked. The context also provides a way to get the toc items (name / ref pair) and will automatically sort them by the vertical position of the ref. If the vertical position matches then it's left to right.
Right now we are allowing more than one. i.e. anything that is in the viewport by a pixel is considered active. This commit does a huge chunk of refactoring under the hood and is not the final destination. It is a step in the right direction. As of this commit the TOC highlights anything in view.
Even while resizing or scrolling!
Not the best UX behavior.
Not trying to match the design, just something we can visiually check against
The way we determine if a TOC item is active is based on a custom algorithm that calculates the percent traveled. If we don't expose a way to scroll an item it's unclear to the implementing developer exactly how the context selection for active works. Now the scroll to logic is exposed and does not require the dev to read the logic. As part of this commit I also added an optional buffer to the scroll to logic. This is required because already have an element which takes up no space, but then does after some amount of scrolling: The scroll indicator. I will look to see if I can remove it from the css flow, but for now this works great.
Some notes here:
This code:
```
marginLeft={-4}
listItemProps={{
paddingLeft: 4,
}}
```
Is to break out fo the container such that we can have the background color
extend to the left of the list item.
The different exports useScrollPercentHeight allow TS to infer the type of the props.
I am working hard to get TOC / TOC Item to be a generic component that can be used
in other places. As such, I am trying to allow props to be passed in to control
the styling of the component at each call site, but have the default styling
make sense.
This is mostly a copy from the one in the main repo, but I spent some time getting the component to work with row or column flex containers.
Instant exists in the docs, but I guess not in our env. I will back out of this choice for now
It seems setState / useEffect is not called server side in nextjs. So we need to render the titles server side. I am thinking I will work to undo some of the more dynamic changes of this TOC Context and instead make it such that the TOC renders server side and then the client side code can take over. Coming soon in near commits: The current thought is to change pushItem to instead always push a new title or throw an error if the title is already in the TOC. Then we can add a new method which will get the TOCRecord for a given title. This will be used to get the element ref for the title. With the two above changes we can render the TOC server side and then have the client side inject the refs. Similarly we can have the client side actually be able to add to the TOC on the fly. Although we will not need to use this feature right now, I am going to keep it on the context for now. Effectively, pushItem will be dead code, but it also may be useful in the future.
The main goal was to simplify the Provider and it's required functions.
Now the TOC has hooks to force serverside rendering and link back up client side as well as a way to add a new item client side. For more explanation see the previous commit message
Default to on in dev / test, but off in prod
jarstelfox
force-pushed
the
scroll-add-ui
branch
from
6347fab to
d163565
Compare
| } | ||
| } | ||
|
|
||
| function getGradientsBaseStyle( |
There was a problem hiding this comment.
It's hard to tell exactly what this component does... but it feels very complex for what seemingly amounts to making a gradient slightly bigger or smaller depending on the browser width. I don't doubt that it looks cool but it strikes me as a lot of complexity (and thus the maintenance) for what it buys us.
There was a problem hiding this comment.
This is copied from code in the monorepo.
It allows you to pull a nice fade/gradient on any scrollable container. The fade changes sizes depending on the space available so that it does not look out of place.
Although, I hear you, we have this implemented in a few different ways in the mono repo and all versions behave differently. This came up when working on the Guide Products V2 project. We had decided as a team to spend effort to create reusable and well-tested components which will solve UI problems for us once and only once.
I can port the full test suite for this component across as well. If that helps?
There was a problem hiding this comment.
This is copied from code in the monorepo.
Ok. Though that doesn't refute the point I was trying to make. Potentially makes it worse as we have at least two copies of this complexity.
Although, I hear you, we have this implemented in a few different ways in the mono repo and all versions behave differently
-< (link!)
I can port the full test suite for this component across as well. If that helps?
I'd rather not. It doesn't help my concerns of complexity.
There was a problem hiding this comment.
When examining how this looks, I think I prefer the look of a fixed-height gradient (or none at all) over one that has a dynamic height.
There was a problem hiding this comment.
I think some more general discussion about the desired fade behavior might be in order.
The other feature taken into account here is that the fade slowly shifts its opacity as you scroll until you pass its end.
Here is what I am talking about:
Screencast.from.2023-08-22.16-59-54.webm
Note: I made the fade red to help visibility.
Here's what it looks like normally
Screencast.from.2023-08-22.17-01-35.webm
There was a problem hiding this comment.
@ianrohde Showed me this approach which does work, but only in chrome and it puts the box shadow behind text, not in front of it: https://daverupert.com/2023/08/animation-timeline-scroll-shadows/
| <ModalHeader>{title}</ModalHeader> | ||
| </VisuallyHidden> | ||
| <ModalCloseButton /> | ||
| <VisuallyHidden></VisuallyHidden> |
There was a problem hiding this comment.
I assume this empty "hidden" component was unintentional?
There was a problem hiding this comment.
I did not add this. The diff only shows this because I added a container and the code shifted inward with whitespace.
There was a problem hiding this comment.
I don't recall adding anything like this.
There was a problem hiding this comment.
It looks like this was added here: 00f6c9f
I think we can clean it up, but I'd love to handle that as a follow up.
|
un_dev_block 🌵 The code is behind a flag. We can safely deploy and iterate on changes. |
There was a problem hiding this comment.
I tried to check through the force-pushes, and most seem reasonable. Looks you pushed away the separate /dev/feature route.
Otherwise these changes seem mostly the same to version we pair-reviewed.
If this is disabled in production, lets get this merged so that we can continue to iterate against master, without such overwhelming diffs.
CR ⚙️
|
QA 🎬
☝️ Refreshing the page causes the progress bar to disappear |
|
When viewing the preview, the TOC list for mobile jumps around a ton while scrolling within it. dev_block 👍 This is especially noticeable on the ("Many small items" example](https://react-commerce-prod-git-scroll-add-ui-ifixit.vercel.app/Troubleshooting/Chainsaw/Overheating/484453) |
|
@danielbeardsley any reason why you dev blocked when the goal was to iterate this in a follow up: #1923 (comment)?
un_dev_block 🌵 but deploy_block 🌵 on a discussion Am I missing something? The env variable is not set in Vercel for the prod app:
Similarly, the production env file is committed to be off upon deploy: |
|
un_deploy_block 🎏 If we're confident this is behind a feature flag that won't be relevant in prod, let's continue and just make sure that we note all of the current issues for follow-up work. |
Nope! Sorry I didn't see that comment! |
I understand that long pulls can get unruly and comment threads can be harder to follow, but chopping them off by merging and wanting discussion to continue someplace other than the diff of the code has its own disadvantages. Threads either get lost, or must continue in a context separate from the changes they are discussing. That said, I'm fine with merging this! |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Some choices made:
CR
There was a lot of back and forth after about the TOC logic after 38c2ab6.
I would CR commit by commit up to there then start looking at the new files TOC / TOCContext as greenfield and look at the overall diff.
QA
react-commercedeploymentreact-commerce-proddeploymentConnects: https://github.com/iFixit/ifixit/issues/49284