Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(docs): Table of Contents component #15251

Merged
merged 11 commits into from
Jul 30, 2019
Merged

feat(docs): Table of Contents component #15251

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
809ff7d
Initial progress on Table of Contents
lannonbr Jun 30, 2019
c81bcda
Apply suggestions from code review
lannonbr Jul 1, 2019
ebbe93e
updating header to h2
lannonbr Jul 1, 2019
e04a8ab
Merging Flo's work in and added frontmatter field to disable TOC
lannonbr Jul 20, 2019
fe93df4
Merge remote-tracking branch 'origin' into docs-table-of-contents
lannonbr Jul 20, 2019
3578bea
Moved h2 into nav component
lannonbr Jul 20, 2019
792dccb
Bring back `sizes` token additions, fixes tests
fk Jul 28, 2019
d623592
Revert changes in `packages/gatsby-design-tokens`
fk Jul 28, 2019
aaf8aae
Prevent ToC content becoming inaccessible
fk Jul 11, 2019
377f653
Merge branch 'master' into docs-table-of-contents
gillkyle Jul 29, 2019
527a9ba
fix: prettier fix and disabling one missing ToC field
gillkyle Jul 29, 2019
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions www/src/components/docs-table-of-contents.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import React from "react"
import { Link } from "gatsby"
import {
fontSizes,
colors,
zIndices,
shadows,
space,
mediaQueries,
} from "../utils/presets"

function createItems(items, location) {
return (
items &&
items.map(item => (
<li>
<Link to={location.pathname + item.url}>{item.title}</Link>
{item.items && (
<ul css={{ marginLeft: space[6] }}>
{createItems(item.items, location)}
</ul>
)}
</li>
))
)
}

function TableOfContents({ page, location }) {
return page.tableOfContents.items ? (
<div
css={{
background: colors.white,
zIndex: zIndices.feedbackWidget,
lannonbr marked this conversation as resolved.
Show resolved Hide resolved

[mediaQueries.xxl]: {
padding: space[6],
boxShadow: shadows.overlay,
lannonbr marked this conversation as resolved.
Show resolved Hide resolved
float: `right`,
marginLeft: space[10],
position: `sticky`,
top: 130,
},
}}
>
<h1
gillkyle marked this conversation as resolved.
Show resolved Hide resolved
css={{
textTransform: `uppercase`,
fontSize: fontSizes[2],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd go for fontSizes[1] here, and throw in a letterSpacing: letterSpacings.tracked—ref. the excellent https://practicaltypography.com/letterspacing.html for why we want extra letterspacing for all caps text.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done
color: colors.grey[60],
}}
>
Table of Contents
</h1>
<nav>
lannonbr marked this conversation as resolved.
Show resolved Hide resolved
<ul>{createItems(page.tableOfContents.items, location)}</ul>
</nav>
</div>
) : null
}

export default TableOfContents
22 changes: 20 additions & 2 deletions www/src/templates/template-docs-markdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React from "react"
import { Helmet } from "react-helmet"
import { graphql } from "gatsby"
import { MDXRenderer } from "gatsby-plugin-mdx"
import { mediaQueries } from "../utils/presets"

import Layout from "../components/layout"
import {
Expand All @@ -11,6 +12,7 @@ import {
} from "../utils/sidebar/item-list"
import MarkdownPageFooter from "../components/markdown-page-footer"
import DocSearchContent from "../components/docsearch-content"
import TableOfContents from "../components/docs-table-of-contents"
import FooterLinks from "../components/shared/footer-links"

import Container from "../components/container"
Expand Down Expand Up @@ -49,11 +51,26 @@ function DocsTemplate({ data, location }) {
enableScrollSync={urlSegment === `docs` ? false : true}
>
<DocSearchContent>
<Container>
<Container
Copy link
Contributor

@fk fk Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just gave this a spin last night locally (insomnia), and noticed that the main content currently sticks to the left of the…"main sidebar nav" (the one to the left, containing the navigation for "Docs/Tutorial/Features")…on large screens where the TOC is positioned to the right of the main content:

image

IMO we want the whole content, including our new TOC sidebar, to stay centered. Not super trivial, but also not too hard to do—so I got tempted, and gave that a quick shot. Pushed my stuff here:
8616bc1
Copy link
Contributor

@fk fk Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's how what is in that PoC branch looks like:

toc-poc-low

It's not perfect (started off with a different approach, so it's not fully fluid yet, container size is "hardcoded" a little more than it needs still, so if the ToC content doesn't need the full width of what the layout container currently reserves for it, things look a little weird (uncentered)), but maybe it helps a little!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • To make it a bit easier to get there (and understand/maintain this stuff in the future), this drops pulling .gatsby-highlight & Co. to the left/right for mediaQueries.lg—no need to worry about this detail IMO. It was fun while it lasted, time to move on. (happened in typography.js)
  • Also I thought it's time to drop defining <Container>s maxWidth as rhythm, and added those values to tokens/sizes as rems.
  • That commit might also include what I mentioned in https://github.com/gatsbyjs/gatsby/pull/15251/files#r302286146 ¯\_(ツ)_/¯ 😉

Infamous last words: We eventually have to take care of the TOC sidebar height, too.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah thank you @fk for this. The container being pushed around felt very hacky when I built it but this is looking to be much better! I will look into this deeper later
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Ben—your work made it really easy for me to jump in to do this! 🙌

Sorry for the sloppy commit there—it looks a bit much, but IMO contains a lot of valuable changes. Please don't hesitate to reach out if something is unclear, or just pick what you think is valuable. One thing I want to trying clarifying is what I meant by

started off with a different approach, so it's not fully fluid yet, container size is "hardcoded" a little more than it needs still, so if the ToC content doesn't need the full width of what the layout container currently reserves for it, things look a little weird (uncentered)

—I think we might not need to define a maxWidth for the outer container at all:

8616bc1#diff-731b5641a3566389a9aa7e6d232fe6adR26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ Ah no, ignore that last bit about maxWidth—I think that PoC is as good as it gets with the <h1> and the requirements defined in #6896 (comment) (ToC following the main headline on smaller screens), at least if we don't want to throw in some JS yada yada, or go for left-aligned content once the ToC on the right kicks in (which isn't too bad, thinking about that again), or revisit the requirements.

Better to worry about content becoming inaccessible if the ToC height exceeds the viewport height IMO. Wondering if we want to avoid two scrollbars relatively close to each other here? An approach as https://react-sticky-box.codecks.io/ looks interesting irt to that.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed a quick rudimentary fix for the latter to https://github.com/gatsbyjs/gatsby/tree/topics/15251-layout (5951933).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This has been merged into my branch.
overrideCSS={{
[page.tableOfContents.items && mediaQueries.xxl]: {
maxWidth: `80rem`,
},
}}
>
<h1 id={page.fields.anchor} css={{ marginTop: 0 }}>
{page.frontmatter.title}
</h1>
<MDXRenderer slug={page.fields.slug}>{page.body}</MDXRenderer>
<TableOfContents location={location} page={page} />
<div
css={{
[page.tableOfContents.items && mediaQueries.xxl]: {
paddingRight: `40rem`,
},
}}
>
<MDXRenderer slug={page.fields.slug}>{page.body}</MDXRenderer>
</div>
{page.frontmatter.issue && (
<a
href={page.frontmatter.issue}
Expand All @@ -80,6 +97,7 @@ export const pageQuery = graphql`
body
excerpt
timeToRead
tableOfContents
fields {
slug
anchor
Expand Down