SideNav Componentization Tracking Issue

emplums commented 5 years ago

This is a tracking/scoping issue for refactoring the SideNav component to be reusable.

Currently the SideNav component is hardcoded to work specifically for the blueprints project docs. I'd like to break it up into several different components to be added to the library. This will require a bit of refactoring to make all the components truly reusable.

Scope of Work

Separate SideNav into 7 components:

SideNav

Container component that takes children and nests them in a Router with layout styling

Section

Can take a path prop and children.
Will only be rendered in the SideNav if the path prop matches the current url.
Children's path will have the parent Section path appended to it's path.
Children provided should ideally be SectionLinks
Renders a NavList if no children are provided, by looking up children of the node the path points too.
Feels a little too abstracted/magical/doing too many things 🤔 Might be better broken up into two different components? not sure because it is super helpful as is

RouteMatch

Used to conditionally show/not show a chunk of content based on a path. For example, a chunk of Section components

NavList

Renders a <SectionLink> for each child of the paths node. For instance - <NavList path='support'/> would render a <SectionLink> for each doc in the pages/support folder

NodeLink

Takes href and children. If children aren't provided then it looks up the meta.title for the href and uses that for the title of the link.
Not 100% sold this is necessary. Feels too magic.

SectionLink

Same as NodeLink, but it renders bold when it's href matches the current path

NavLink

Same as NodeLink, but it renders with black text when it's href matches the current path

Notes

I like the functionality that these components provide, but I'm worried they're too tied to the file structure in next.js being perfect - feels sort of rails-esque. I'm currently considering if a different approach might make them more universal for anyone using React but not necessarily using Next.
Naming could probably be cleaned up a bit. Especially between SectionLink, NodeLink, and NavLink. Do each of these components differentiate based on where they should be used, or how they change when the path matches their href?

shawnbot commented 5 years ago

Yay, happy to see this happening! Please don't feel like you have to keep any of these component names... NodeLink might have made sense for me, but it may not for anyone else. 😂 Maybe we call them PageLink, etc.?

Also, I think I may have discovered a better way to get the list of pages in Next.js, which should eliminate all of the pages hackiness that's happening.

emplums commented 5 years ago

Oooh I would be very interested in hearing more!

shawnbot commented 5 years ago

After hunting around GitHub and Spectrum, I found this thread which suggests that contexts can share data between server and client. If that's the case, then I think it looks something like this:

// pages/_document.js
import NextDocument from 'next/document'
import Pages from '../docs/pages'

export default Document extends NextDocument {
  render() {
    const {buildManifest} = this.props
    return (
      <Pages.Provider value={buildManifest.pages}>
        ...
      </Pages.Provider>
    )
}

// docs/components/SideNav.js
import Pages from '../docs/pages'

export default props => (
  <Pages.Consumer>
    {pages => (
      // do something with the pages object
    )}
  </Pages.Consumer> 
)

...and I guess only the components that need access to it would use <Pages.Consumer>?

If contexts don't work across the server/client boundary, then we might need to wire up a slightly hackier solution, but it's got to be better than what we have now. 😬

shawnbot commented 5 years ago

Maybe an HOC that wraps withRouter() would be nice:

export function withPages(Component) {
  return withRouter(props => (
    <Pages.Consumer>{value => <Component pages={value} {...props} />}</Pages.Consumer>
  ))
}

emplums commented 5 years ago

Oh that is super cool! Totally forgot we could use react context for this 🤔 Since this would be a bit of a refactor, I'll make an issue for it for safe keeping!

primer / blueprints