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

Use scrollspy on docs pages #33428

Merged
merged 5 commits into from
Dec 29, 2022
Merged

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Mar 22, 2021

closes #30977

Maybe can have better choices on css and classes

Preview: https://deploy-preview-33428--twbs-bootstrap.netlify.app/docs/5.2/getting-started/introduction/

@GeoSot GeoSot requested a review from a team as a code owner March 22, 2021 21:52
@GeoSot GeoSot added the docs label Mar 22, 2021
@GeoSot GeoSot marked this pull request as draft March 22, 2021 23:09
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch 2 times, most recently from 6f99f9e to aaad5be Compare March 22, 2021 23:23
@GeoSot GeoSot marked this pull request as ready for review March 22, 2021 23:30
@XhmikosR
Copy link
Member

Oh, nice!

I'll check it out after beta3 to see what we can improve (not a huge fan of the replacements, but I guess there's no other solution right now since Hugo doesn't allow us to customize the ToC template). Also, sort of looks like it's a few pixels off.

I'm pretty sure @mdo will tweak the CSS as needed because currently it looks a little weird 😛

@XhmikosR XhmikosR marked this pull request as draft March 30, 2021 05:06
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from aaad5be to 3d460e7 Compare May 11, 2021 12:35
@mdo mdo self-assigned this May 14, 2021
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 3d460e7 to 0f2c5ce Compare June 3, 2021 23:49
@mdo mdo force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 0f2c5ce to b02e142 Compare June 24, 2021 20:29
@mdo
Copy link
Member

mdo commented Jun 24, 2021

Redesigned things a bit to add a hover state and restore the v3-ish sidebar highlighting. Here's an example of nested active items plus a hovered item down the page.

@GeoSot Is it possible to implement an offset for the scrollspy anchors? Our sticky subnav gets in the way I think, so when I click on the ToC item, it doesn't get highlighted until I scroll.

Screen Shot 2021-06-24 at 1 28 03 PM

@mdo
Copy link
Member

mdo commented Jun 24, 2021

Btw, I'm game for this in v5.1 (or v5.1.1) or v5.2 depending on everyone else :).

@mdo mdo marked this pull request as ready for review June 24, 2021 20:51
@GeoSot
Copy link
Member Author

GeoSot commented Jun 24, 2021

Btw, I'm game for this in v5.1 (or v5.1.1) or v5.2 depending on everyone else :).

Glad to hear it, cause I cannot do much more on the current implementation 😢

I 'll give it a try but my real hope is on Intersection observer #33421

@XhmikosR
Copy link
Member

@mdo on the CSS side, can't we have a transparent left border in the ToC items so that they don't jump when the active class is added?

@ffoodd
Copy link
Member

ffoodd commented Jul 12, 2021

We may also use background to highlight, as we do in ScrollSpy doc, or play with a pseudo-element to allow more design possibilities.

BTW I really like it, thanks @GeoSot :)

@mdo
Copy link
Member

mdo commented Jul 25, 2021

The accuracy of the highlighting feels a little off in at least Safari for me. Anything we can do to help with that?

@XhmikosR XhmikosR marked this pull request as draft July 29, 2021 13:32
@mdo
Copy link
Member

mdo commented Nov 3, 2021

@mdo on the CSS side, can't we have a transparent left border in the ToC items so that they don't jump when the active class is added?

This is a little stale, but this was how I implemented it already :).

@mdo
Copy link
Member

mdo commented Nov 3, 2021

Any chance we can check in on this one though?

@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from b02e142 to 43d9219 Compare February 8, 2022 23:12
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 43d9219 to 9e39327 Compare April 7, 2022 11:30
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 9e39327 to b84d966 Compare April 14, 2022 11:40
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from b84d966 to b95a1a4 Compare April 18, 2022 07:39
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from b95a1a4 to 2ac2fb2 Compare May 15, 2022 23:21
@mdo mdo force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 2ac2fb2 to a4a0c05 Compare June 4, 2022 16:18
@mdo
Copy link
Member

mdo commented Jun 14, 2022

Played with this more locally and it's a little off—sections are getting skipped. Is there anything else to improve here or in the scrollspy implementation itself?

@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch 2 times, most recently from dfdebd7 to a983f6f Compare October 7, 2022 21:32
@GeoSot GeoSot force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from a983f6f to ce2015e Compare October 26, 2022 21:53
Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this work! Is there a chance to see it live in Bootstrap anytime soon ?

I don't know if the design is up to date but it looks fine to me !

The only issues I can see so far are due to the scrollspy component itself so I think it could be merged as-is ?

site/assets/scss/_toc.scss Outdated Show resolved Hide resolved
site/assets/scss/_toc.scss Outdated Show resolved Hide resolved
@@ -1,3 +1,4 @@
{{ define "body_override" }}<body data-bs-spy="scroll" tabindex="0" data-bs-target="#TableOfContents">{{ end }}
Copy link
Member

Choose a reason for hiding this comment

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

I think that there should be a block {{ if (eq .Page.Params.toc true) }} around to avoid trigger the js when there's no need.

That said, I wonder if these attributes should be on .bd-main, .bd-content or keep it here ?

@julien-deramond
Copy link
Member

julien-deramond commented Dec 17, 2022

Some sections are off indeed but it's more likely coming from the Scrollscpy component itself.
Really like what brings this PR in terms of UX.
From what I have tested so far, I think we can deactivate this feature under 991px (when the menu is no more on the right hand side).

@mdo mdo marked this pull request as ready for review December 29, 2022 05:08
@mdo mdo force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 523b75b to 637a112 Compare December 29, 2022 05:08
@mdo mdo force-pushed the gs-use-scrollspy-on-docs-table_of_contents branch from 413999b to 03b5528 Compare December 29, 2022 19:37
@mdo mdo merged commit a9810ec into main Dec 29, 2022
@mdo mdo deleted the gs-use-scrollspy-on-docs-table_of_contents branch December 29, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Use scrollspy on docs pages
6 participants