Skip to content

open sidebar#297

Merged
Fil merged 15 commits intomainfrom
fil/sidebar-open
Dec 3, 2023
Merged

open sidebar#297
Fil merged 15 commits intomainfrom
fil/sidebar-open

Conversation

@Fil
Copy link
Contributor

@Fil Fil commented Dec 1, 2023

Open questions:

  • apply to the cli website or not?
  • don't let a locally stored preference close a menu that contains the current path?

Done:

@Fil Fil marked this pull request as ready for review December 1, 2023 13:35
@Fil
Copy link
Contributor Author

Fil commented Dec 1, 2023

Whether we want to apply it to the cli documentation is debatable; I've applied it here so it's easier to test.

@Fil Fil enabled auto-merge (squash) December 1, 2023 18:11
@Fil
Copy link
Contributor Author

Fil commented Dec 2, 2023

There's a bit of a conflict when you have manually closed a section, then go to a page that belongs to that section. In that case should the section be open (as in #159) or closed (as in #181)?
Currently it's closed, but we might want to give priority to opening?

@mbostock
Copy link
Member

mbostock commented Dec 2, 2023

If you navigate to a page, the section should be open even if you previously closed it; #159 is higher priority than #181.

@mbostock
Copy link
Member

mbostock commented Dec 3, 2023

There’s a related problem here when the sidebar is taller than the window: when you navigate to a page, the sidebar scrolls to the top. The most natural solution would be to use client-side navigation, which would also solve the problem of remembering which sections are open or closed. But to continue the path in this PR, perhaps we could also stash the scroll offset of the sidebar prior to navigating, so that we can restore it when the next page loads?

This also makes me realize that we should be using sessionStorage instead of localStorage to track which sections are open or closed — you only want it to affect the current tab, not other windows you may have open.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

We can add scroll persistence in a followup PR.

@Fil Fil merged commit 1d5d224 into main Dec 3, 2023
@Fil Fil deleted the fil/sidebar-open branch December 3, 2023 22:32
}
function toggleDetails(event) {
if (event.detail > 1) event.preventDefault(); // Prevent double-clicking the summary toggle from selecting text.
sessionStorage.setItem(`observablehq-sidebar:${this.textContent}`, String(!this.parentElement.open));
Copy link
Member

Choose a reason for hiding this comment

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

Oops, we shouldn’t piggyback on the mousedown event here. We should listen for the toggle event on the parent details instead. I’ll fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 83ef581

followup 4434cd0

(thank you!)

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.

Remember which panels are open or closed in the nav Sidebar sections should open automatically when a page within is active

2 participants