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

Addon-docs: Add opt-in table of contents #23142

Merged
merged 14 commits into from
Jul 6, 2023
Merged

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 20, 2023

Closes N/A

What I did

Added opt-in TOC to docs page to address the sidebar navigation issues caused by the 7.0 docs changes.

This is the "built-in" version of https://github.com/storybookjs/addon-toc/pull/1

Added:

  • docs.toc parameter
  • Example stories to demonstrate the key configuration parameters

How to test

  1. Spin up a sandbox.
  2. Browse to the added addon-docs/toc stories which demonstrate different configurations
  3. Add docs.toc = { } parameter to .storybook/preview.js to see how it looks for all the pages

@socket-security
Copy link

socket-security bot commented Jun 20, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@socket-security
Copy link

socket-security bot commented Jun 20, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
tocbot 4.21.0 network, environment +0 807 kB tscanlin

@ndelangen ndelangen self-assigned this Jun 20, 2023
@ndelangen ndelangen requested a review from JReinhold June 20, 2023 15:01
@kylegach
Copy link
Contributor

kylegach commented Jun 21, 2023

Love this, and I think a lot of users will too. ❤️

Strangely, when I spun up a sandbox (yarn task --task sandbox --template react-vite/default-ts) the TOC was rendered despite seemingly no opting-in in either preview.ts or any of the CSF files (i.e. I didn't modify anything and it was there). 🤷‍♂️

I also have some UI-related feedback:

  1. Rather than hide for narrow viewports, could we render it under the component description in a <details>? So it's still usable, but doesn't take up a bunch of vertical room unless expanded. I can provide that code, if helpful.

  2. The "highlight the active item" logic is a bit buggy in my testing. It seems to work OK when just scrolling. But while clicking an item correctly scrolls to it in the viewport, it doesn't highlight most of the time. If we can't polish it right now, I'd prefer we simply disable the behavior entirely, if possible.

@kylegach
Copy link
Contributor

Oh, and a small process meta note: We use the "Docs" PR title area for documentation updates. Would "Addon docs" be better here, to differentiate? Should documentation updates use something else?

@shilman shilman changed the title Docs: Add opt-in Table of Contents Addon-docs: Add opt-in Table of Contents Jun 21, 2023
@ndelangen ndelangen marked this pull request as ready for review June 27, 2023 17:23
@ndelangen
Copy link
Member

@shilman I marked this ready for review, do you agree we can move this forwards?

@JReinhold
Copy link
Contributor

JReinhold commented Jul 3, 2023

Can we add stories for this? Maybe that's a bit complicated, but then at least some MDX files that shows it off. (maybe in a separate PR)

@shilman shilman changed the title Addon-docs: Add opt-in Table of Contents Addon-docs: Add opt-in table of contents Jul 4, 2023
@shilman
Copy link
Member Author

shilman commented Jul 5, 2023

@JReinhold I added stories to the PR

@kylegach I fixed the bug where TOC is always enabled. Open to UI improvements to this -- we're just using the default behavior of tocbot and I feel it's quite rough. I'm not sure whether it's our usage or whether that's just the fault of the library.

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Everything looks good, only minor comments. 💪

@kylegach

Rather than hide for narrow viewports, could we render it under the component description in a <details>? So it's still usable, but doesn't take up a bunch of vertical room unless expanded. I can provide that code, if helpful.

I agree this would be an interesting improvement if we can get the design right, but it shouldn't block this initial implementation

The "highlight the active item" logic is a bit buggy in my testing. It seems to work OK when just scrolling. But while clicking an item correctly scrolls to it in the viewport, it doesn't highlight most of the time. If we can't polish it right now, I'd prefer we simply disable the behavior entirely, if possible.

This buggy behaviour is pretty noticeable in our case, because story headings often don't fully scroll to the top. From the tocbot code it looks like this is just how it works and there's no way to configure it: https://github.com/tscanlin/tocbot/blob/master/src/js/build-html.js#L179-L197

There might be hacky workarounds for this, but no easy fix.

Comment on lines +11 to +13
* When true, hide the TOC. We still show the empty container
* (as opposed to showing nothing at all) because it affects the
* page layout and we want to preserve the layout across pages.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand this correctly, that if a user has toc in project annotations, and for a single component have disable, we always show the TOC container (but sometimes empty), so that layout won't shift?

But if the user doesn't have toc set global, but just for a single story, then we'll have layout shifts?

I think this is a good solution, I just want to make sure my understanding is correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so. This is @cdedreuille 's comment I believe.

code/ui/blocks/src/components/TableOfContents.tsx Outdated Show resolved Hide resolved
Co-authored-by: Jeppe Reinhold <jeppe@chromatic.com>
@shilman shilman merged commit fbd6bf8 into next Jul 6, 2023
@shilman shilman deleted the shilman/toc-proof-of-concept branch July 6, 2023 09:37
@github-actions github-actions bot mentioned this pull request Jul 6, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants