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.

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

feat(docs): Table of Contents component #15251

Merged
merged 11 commits into from Jul 30, 2019

Conversation

@lannonbr
Copy link
Member

commented Jun 30, 2019

Description

Added a Table of Contents (TOC) component to pages in the docs that separate parts of the page using headings. This is all based upon discussions in #6896 and pair programming sessions I had with @marcysutton and @shannonbux.

TODO

These things need to be resolved before getting merged:

  • Make sure this is accessible
  • In the template for the docs, I am adding some styling to some various components to expand the container so on most pages on a large display, the TOC won't overlap as it scrolls down the page. I think this can likely be handled better.
  • Clean up some of the design with advice from @fk. I tried using some of the design tokens now set up in src/utils/presets.js

As well, as I have been going through some pages to test this out, some things have appeared to me. Some of these can likely be tackled as separate PRs.

  • some pages have a TOC already that is static. We could remove the static TOC.
  • We may want to disable the TOC on some pages. For instance, on the Glossary page we may want to use the alphabet component as a TOC instead of this TOC. We could maybe have a custom field in the frontmatter to disable this TOC.

Screenshots

Here's a few screenshots of the current state:

On large screens, there now will be a floating component on the right that displays the Table of Contents. This uses position: sticky so it will stick to the viewport as the page is scrolled down.

GraphQL Reference page with TOC on right for large display

On mobile and small screens, it will be in the flow of the regular document:

Introducing GraphQL page with TOC inline on mid-sized display
Introducing GraphQL page with TOC inline on phone-sized display

Related Issues

Fixes #6896

@lannonbr lannonbr requested review from fk, marcysutton and shannonbux Jun 30, 2019

@lannonbr lannonbr requested a review from gatsbyjs/website as a code owner Jun 30, 2019

@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Jun 30, 2019

@gatsbyjs gatsbyjs deleted a comment from gatsbot bot Jun 30, 2019

@gillkyle
Copy link
Contributor

left a comment

Woah! This is awesome 🎉 Thanks so much for working on this!

I left a couple suggestions...

And based off of Marcy's acceptance criteria in the issue (#6896) there's probably a couple more things to add but I'm excited for this.

www/src/components/docs-table-of-contents.js Outdated Show resolved Hide resolved
www/src/components/docs-table-of-contents.js Outdated Show resolved Hide resolved
www/src/components/docs-table-of-contents.js Outdated Show resolved Hide resolved
@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

@lannonbr I want to second everything @gillkyle said — this is awesome!

lannonbr and others added some commits Jul 1, 2019

Apply suggestions from code review
Co-Authored-By: gillkyle <kylerobertgill@gmail.com>
@marcysutton
Copy link
Member

left a comment

This is awesome, and so simple! I agree that we'll want a way to disable it on some pages. Any ideas for a frontmatter key (tableOfContents: false)?

www/src/components/docs-table-of-contents.js Outdated Show resolved Hide resolved
@shannonbux

This comment has been minimized.

Copy link
Collaborator

commented Jul 9, 2019

@marcysutton

This comment has been minimized.

Copy link
Member

commented Jul 10, 2019

the design could use colors that are a little softer

As long as the colors meet contrast requirements for regular text, sounds like a good design decision to me!

@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

☝️👍

Contrast scores also are available in the modal for each color group on https://www.gatsbyjs.org/guidelines/color/ btw.

<h2
css={{
textTransform: `uppercase`,
fontSize: fontSizes[2],

This comment has been minimized.

Copy link
@fk

fk Jul 10, 2019

Contributor

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.

This comment has been minimized.

Copy link
@lannonbr

lannonbr Jul 20, 2019

Author Member

Done

fk added a commit that referenced this pull request Jul 11, 2019

@@ -49,11 +51,26 @@ function DocsTemplate({ data, location }) {
enableScrollSync={urlSegment === `docs` ? false : true}
>
<DocSearchContent>
<Container>
<Container

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor

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!

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor
  • 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.

This comment has been minimized.

Copy link
@lannonbr

lannonbr Jul 11, 2019

Author Member

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

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor

^ 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.

This comment has been minimized.

Copy link
@fk

fk Jul 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@lannonbr

lannonbr Jul 21, 2019

Author Member

Done. This has been merged into my branch.

@lannonbr

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Just a notice I will likely continue work on this PR on Thursday. Been busy with other things this week but the end of the week is much lighter

@shannonbux
Copy link
Collaborator

left a comment

I made comments a couple weeks ago with my full review! Just making sure to do this so github doesn't keep reminding me to leave a review :)

lannonbr and others added some commits Jul 20, 2019

Merging Flo's work in and added frontmatter field to disable TOC
Co-authored-by: Florian Kissling <21834+fk@users.noreply.github.com>

@lannonbr lannonbr requested review from gatsbyjs/core as code owners Jul 20, 2019

@lannonbr

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

Alright, so I continued some work with this. I merged in the changes @fk made in a side branch so the container stuff is a bit more succinct and the design fits better.

With what @marcysutton commented for the disabling of the table of contents, I added a disableTableOfContents field in the frontmatter that when set to be true will not render this TOC and just use the one that may exist already on the page. I added this to the glossary page as an example but we can do a scan of other pages at a later point of if we want to disable it on other pages.

There seems to be some issues with the www unit tests. I will try debugging those later this weekend.

@lannonbr lannonbr removed the status: WIP label Jul 20, 2019

@marcysutton

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

I just saw that same error locally that's coming up on Circle unit_tests_www actually: Error: Uncaught [TypeError: Cannot read property 'withSidebar' of undefined]

@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

@lannonbr @marcysutton Tests are failing because when merging master in fe93df4, git got a little too smart and moved ww/src/utils/tokens/sizes.js to packages/gatsby-design-tokens/src/sizes.js, and the lines now causing problems with the tests (

mainContentWidth: {
default: `54rem`,
withSidebar: `42rem`,
},
) with it.

For this to work, we'd need to publish gatsby-design-tokens first.

That's what we (I 😉) get for moving the design tokens into their own package, and including stuff in there which shouldn't have been there in the first place, namely the sizes tokens that we added mainContentWidth to.

I'll push a fix. ✌️

fk added some commits Jul 28, 2019

Prevent ToC content becoming inaccessible
…when exceeding the viewport height via `max-height` and `overflow: auto`
@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

@lannonbr

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2019

Thanks so much for the fixes @fk!

@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2019

@lannonbr You're welcome! 🤗

@lannonbr

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

This should be ready for another review

@amberleyromo
Copy link
Member

left a comment

Took a quick look and it looks great to me! @gillkyle's gonna take one last look :)

Merge branch 'master' into docs-table-of-contents
# Conflicts:
#	www/src/templates/template-docs-markdown.js

@lannonbr lannonbr requested a review from gatsbyjs/themes-core as a code owner Jul 29, 2019

@gillkyle

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Just merged master to resolve problems with the prev and next buttons that got merged recently. Also removed the Table of Contents that were on pages that I could find doing a quick couple of regex searches across the repo and disabled the ToC on the tutorial where the ToC is a little redundant since that the majority of that info is already displayed in the sidebar.

Looks like there are some linting errors now that I'll fix and then merge.

@gillkyle gillkyle merged commit 1a54d1f into master Jul 30, 2019

8 checks passed

Danger All good
Details
Gatsby Build Service Gatsby Build Service
Details
Peril All green. Congrats.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
unit_tests_windows Build #20190729.141 succeeded
Details

@gillkyle gillkyle deleted the docs-table-of-contents branch Jul 30, 2019

@gillkyle

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

Thanks for all the awesome work on this everyone! @lannonbr, @shannonbux, @marcysutton, @fk 🎉

@fk

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

🎉 Dito, and thank you Kyle!

johno added a commit to johno/gatsby that referenced this pull request Aug 2, 2019

feat(docs): Table of Contents component (gatsbyjs#15251)
* Initial progress on Table of Contents

* Apply suggestions from code review

Co-Authored-By: gillkyle <kylerobertgill@gmail.com>

* updating header to h2

* Merging Flo's work in and added frontmatter field to disable TOC

Co-authored-by: Florian Kissling <21834+fk@users.noreply.github.com>

* Moved h2 into nav component

* Bring back `sizes` token additions, fixes tests

* Revert changes in `packages/gatsby-design-tokens`

* Prevent ToC content becoming inaccessible

…when exceeding the viewport height via `max-height` and `overflow: auto`

* fix: prettier fix and disabling one missing ToC field
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.