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

Table of Contents: Add toggle #389

Merged
merged 12 commits into from
May 11, 2023

Conversation

alexstine
Copy link
Contributor

Adds a toggle for the Table of Contents block.

See WordPress/wporg-documentation-2022#58 for additional context.

@alexstine
Copy link
Contributor Author

@ryelle Here is the basic code but not sure how to inject JavaScript on the front-end for this. Is there a way with blocks or do I just need to call wp_register_script(), wp_enqueue_script()?

@ryelle
Copy link
Contributor

ryelle commented Apr 10, 2023

There's already a front end script for this block, it's at table-of-contents/src/view.js, currently it just handles the positioning to make the sidebar stay stuck to the screen while scrolling the page. If we move away from that layout (of having a sidebar) we can probably remove most of that code… but it will depend on the design. If you implement the toggle here, I can work with @jasmussen on how it should look 👍🏻

@alexstine
Copy link
Contributor Author

@ryelle Got it working.

@alexstine alexstine marked this pull request as ready for review April 11, 2023 03:03
@ryelle
Copy link
Contributor

ryelle commented May 5, 2023

I've updated this PR with the styles suggested by @fcoveram. The table of contents is collapsed by default so that it's easy to skip. It's collapsed at all sizes, even when it's shifted to the sidebar, so that it is still skipped for screen reader users. This makes for kind of an awkward visual, with the only thing in the sidebar a toggle button. The alternative would be to have the table of contents expanded by default when the screen is wide enough (1200px). The list would still be collapsible, so a screen reader user can collapse it to get by. Would that be acceptable, @alexstine?

Screenshots for design review:

Collapsed (default) Expanded
Small screen (450)
Mid-sized screen (960)
Large screen (1400)

@ryelle ryelle self-assigned this May 5, 2023
@ryelle ryelle requested review from StevenDufresne, adamwoodnz and a team May 5, 2023 23:15
@jasmussen
Copy link
Collaborator

Looks good to me. My only question would be whether we can have the table of contents be expanded by default, if you're on a large (two columns) screen.

@ryelle ryelle force-pushed the add/table-of-contents-toggle branch from 8f61c31 to ecdf697 Compare May 8, 2023 19:21
Copy link
Collaborator

@fcoveram fcoveram left a comment

Choose a reason for hiding this comment

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

I notice an unbalance spacing in the sides. Sorry that I can't point out specific values and what might cause them, but here is a screenshot highlighting the areas.

TOC component in small and large screen viewports. Paddings of the component are highlighted

@ryelle
Copy link
Contributor

ryelle commented May 9, 2023

@fcoveram Is that what it currently is, and it should be different? Can you tell me the values each side should be?

@alexstine
Copy link
Contributor Author

Collapsed or expanded by default is fine, whichever way works better visually.

Thanks.

@fcoveram
Copy link
Collaborator

@ryelle I designed it with 15px for top and bottom paddings and 20px for left and right. But I understand that the outcome doesn't necessarily match Figma design, so adding a mockup with the spacing guides for reference.

Mockup of TOC component with spacing guides for the sides

@ryelle
Copy link
Contributor

ryelle commented May 10, 2023

@fcoveram I think you're looking at the production site, the branch here has cleaned up some of those spacing issues. I just fixed it up a bit more so it should match your design now.

Screenshot 2023-05-10 at 4 47 14 PM

(once this is merged, the templates in documentation & developers will need to updated to remove the attributes, since they're not necessary)

@ryelle
Copy link
Contributor

ryelle commented May 10, 2023

Looks good to me. My only question would be whether we can have the table of contents be expanded by default, if you're on a large (two columns) screen.

Collapsed or expanded by default is fine, whichever way works better visually.

Okay, I've set it to be expanded by default when the screen is large enough for the table of contents to be visually in the sidebar, otherwise it's collapsed by default. The toggle is there for all sizes.

@fcoveram
Copy link
Collaborator

It looks great, thanks @ryelle. I was looking at the images you shared in this message 🤔 . Nonetheless, the last update works excellent.

@fcoveram fcoveram self-requested a review May 11, 2023 07:41
@ryelle ryelle merged commit 4af24b4 into WordPress:trunk May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants