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

Add "On this page" TOC #628

Merged
merged 6 commits into from
Nov 16, 2023
Merged

Add "On this page" TOC #628

merged 6 commits into from
Nov 16, 2023

Conversation

kylegach
Copy link
Collaborator

@kylegach kylegach commented Nov 16, 2023

What I did

  • Fix non-fluid DocsLayout
  • Fix scroll margin of headings and code snippets
  • Ensure code in content doesn't overflow column
  • Remove drop shadow on YouTubeCallout
  • Add "On this page" TOC
    • Add InPageTOC component
    • Query tableOfContents in DocsScreen
    • Add right rail to DocsScreen when wide enough to accommodate it

Wide viewports

Note

Currently, this has to be quite wide (1548px) for the main content to be wide enough to be legible. (The screenshot was taken at 1800px wide.) We need to discuss the overall page layout to improve this. See related code comment..

How to test

Note

The broken sidebar links are a known, unrelated issue.

  1. Open the deploy preview
  2. Navigate to a page with lots of headings, like the argTypes API reference
  3. Confirm the "On this page" feature renders correctly
    1. At viewports > 1548px, it should be expanded, in the right rail
      1. Also confirm that the overall page layout looks correct, with no overflowing content
      2. Click one of the links
        1. Confirm that you are scrolled to the correct heading and that it appears visibly, just under the sticky header
      3. Find a page with lots of headings (argTypes API reference is a good one)
        1. Shrink your browser's viewport vertically until the full "On this page" section can't fit
          1. Confirm that the scrolling works as expected
    2. At viewports <= 1548px, it should not render
  4. Navigate to a page with less than 4 headings, like the Install page
    1. Confirm that the "On this page" feature is not rendered

Copy link

netlify bot commented Nov 16, 2023

Deploy Preview for storybook-frontpage ready!

Name Link
🔨 Latest commit 5411a51
🔍 Latest deploy log https://app.netlify.com/sites/storybook-frontpage/deploys/65565ced4ee9b200079ca3c3
😎 Deploy Preview https://deploy-preview-628--storybook-frontpage.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@kylegach kylegach force-pushed the refresh/on-this-page branch from bf66726 to fc7848f Compare November 16, 2023 05:10
@cdedreuille
Copy link
Contributor

cdedreuille commented Nov 16, 2023

@kylegach I think you could remove the TOC if the main content in the middle is below 600px. TOC is mostly useful when there's space. Otherwise we just hide it.

- Add InPageTOC component
    - Can be rendered as a `details` (for narrow contexts)
- Query `tableOfContents` in DocsScreen
- Add right rail to DocsScreen when wide enough to accommodate it
@kylegach kylegach force-pushed the refresh/on-this-page branch from fc7848f to 92d08e8 Compare November 16, 2023 18:11
- Remove collapsed functionality from InPageTOC
- Dramatically simplify DocsScreen layout
    - No longer need to shift InPageTOC from between title & content to right rail
@kylegach kylegach force-pushed the refresh/on-this-page branch from 92d08e8 to 5411a51 Compare November 16, 2023 18:18
@kylegach kylegach marked this pull request as ready for review November 16, 2023 18:46
Copy link
Contributor

@cdedreuille cdedreuille left a comment

Choose a reason for hiding this comment

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

LGTM!

@kylegach kylegach merged commit eb22118 into docs-design-refresh Nov 16, 2023
6 checks passed
@kylegach kylegach deleted the refresh/on-this-page branch November 16, 2023 18:55
@kylegach kylegach mentioned this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants