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

Make the whole left sidebar scrollable #502

Merged
merged 2 commits into from
Oct 17, 2021

Conversation

OriolAbril
Copy link
Contributor

Tried to fix #500. It seems to be working based on a couple local tests,
but all I know about html and css I have learned from looking at sphinx/jekyll
templates and themes, so I am wildly unaware of extra unintended consequences
that might come with the change.

I have also tried to read the contributing section but haven't been able to
figure out if I have to write tests nor in case I do how should I go about
that. Guidance will be very appreciated.

@OriolAbril
Copy link
Contributor Author

OriolAbril commented Oct 16, 2021

Looking at the preview, the vertical scrollbar does appear when expanding all the subsections in the demo site for example but a horizontal one appears now in all pages which should not be the case.

This did not happen in the couple local tests I did which I am not realizing all have the searchbar on the navbar, not on the sidebar, so it probably has to do with some bad interactions/clashes between the searchbar template and the nav scrollable section added here to englobe it.

@choldgraf
Copy link
Collaborator

choldgraf commented Oct 17, 2021

good catch - I think you could delete:

@supports (position: -webkit-sticky) or (position: sticky) {
max-height: calc(100vh - 11rem);
overflow-y: auto;
}

along with #500 (comment)

and then this would remove that second sidebar...does that work?

@OriolAbril
Copy link
Contributor Author

Working like a charm locally, thanks for the help!

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This LGTM - thanks for this improvement!

@choldgraf choldgraf merged commit 8c998eb into pydata:master Oct 17, 2021
@OriolAbril OriolAbril deleted the scroll-sidebar branch October 17, 2021 14:53
@jorisvandenbossche
Copy link
Member

I think this change might have broken the "center/remember scrolling position in the sidebar" feature (not fully sure to be clear, I didn't actually bisect exactly, but this seems the only related commit since the last release).

To see the issue, you can compare https://pydata-sphinx-theme.readthedocs.io/en/latest/demo/subpages/subsubpages/subsubpage18.html (latest docs of master) vs https://pydata-sphinx-theme.readthedocs.io/en/stable/demo/subpages/subsubpages/subsubpage18.html (docs of last release). In the second case, it puts the active item in the navigation "in sight", while on master it falls off the page.

@jorisvandenbossche
Copy link
Member

FYI that feature was added in #363, so this is done by some javascript code:

// Navigation sidebar scrolling to active page
function scrollToActive() {
var sidebar = document.getElementById('bd-docs-nav')
// Remember the sidebar scroll position between page loads
// Inspired on source of revealjs.com
let storedScrollTop = parseInt(sessionStorage.getItem('sidebar-scroll-top'), 10);
if (!isNaN(storedScrollTop)) {
sidebar.scrollTop = storedScrollTop;
}
else {
var active_pages = sidebar.querySelectorAll(".active")
var offset = 0
var i;
for (i = active_pages.length - 1; i > 0; i--) {
var active_page = active_pages[i]
if (active_page !== undefined) {
offset += active_page.offsetTop
}
}
offset -= sidebar.offsetTop
// Only scroll the navbar if the active link is lower than 50% of the page
if (active_page !== undefined && offset > (sidebar.clientHeight * .5)) {
sidebar.scrollTop = offset - (sidebar.clientHeight * .2)
}
}
// Store the sidebar scroll position
window.addEventListener('beforeunload', () => {
sessionStorage.setItem('sidebar-scroll-top', sidebar.scrollTop);
});
}

It might be a matter of updating the div id that is used to determine the scrolling position (as it is now the parent div that scrolls)

@choldgraf
Copy link
Collaborator

choldgraf commented Oct 26, 2021

Ah i bet you are right! Because the thing that is scrolling is no longer just the toc div, but the whole sidebar div. I bet the selector for the"remember position" stuff is selecting for the toc div

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.

Only the toctree section of the left sidebar is scrollable
3 participants