Tweak sidebar to update navigation highlight on scroll #48
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Deploy preview ready! Built with commit 39def4b |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
| const itemIds = _getItemIds(section.items); | ||
| this.setState({ | ||
| itemTopOffsets: _getElementTopOffsetsById(itemIds), | ||
| }); |
There was a problem hiding this comment.
Offsets can change when the window is resized. This approach would require recomputing offsets after a "resize" event.
| {section.items.map(item => ( | ||
| <li | ||
| key={item.id} | ||
| class Section extends Component { |
There was a problem hiding this comment.
How do you feel about leaving the Section component as the simple, functional component it was before and creating a new component that decorates it and adds this highlighting behavior? I think this might provide a couple of benefits:
Sectionstays simple and focused on markup.- Pages like Tutorial could opt into using this scroll-sync decorator while other pages like Docs can skip it and avoid the unnecessary performance cost.
src/utils/createLink.js
Outdated
| isActive, | ||
| }) => { | ||
| const active = | ||
| typeof isActive === 'boolean' ? isActive : isItemActive(location, item); |
There was a problem hiding this comment.
Would be nice if we didn't have to do this in all of the createLink* util methods.
What do you think about just making isActive a boolean parameter that's always expected, and:
- In the case of the scroll-sync decorator component, we could do a smarter check and pass along as a prop to
Section. - If no such prop is passed then
Sectioncan just call the defaultisItemActiveutil itself
|
Thanks for the feedback @bvaughn! 🙏 Will make this changes later today! |
|
@bvaughn - what do you reckon? |
| @@ -18,9 +18,7 @@ import isItemActive from 'utils/isItemActive'; | |||
| import slugify from 'utils/slugify'; | |||
| import {colors, media} from 'theme'; | |||
There was a problem hiding this comment.
The isItemActive import can be removed now that it's no longer being used.
* ReactDOM page translation * Fixing translation Co-Authored-By: michellocana <[email protected]> * Fixing hydrate section translation Co-Authored-By: michellocana <[email protected]> * Fixing browser support section translation * Component -> Componente
This intends to fix #21.
Basically, as the page scrolls or the hash is changed, it will update the 'active' sidebar nav item accordingly.