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.

Sign up for GitHub

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

Jump to bottom

🔍 Make document outline collapsible #373

Merged
merged 16 commits into from
Sep 24, 2024
Merged

🔍 Make document outline collapsible #373

merged 16 commits into from
Sep 24, 2024

Conversation

rowanc1
Copy link
Collaborator

@rowanc1 rowanc1 commented May 6, 2024 •
edited by agoose77
Loading

Adds new collapsible control to hide document outline when elements intersect with the margin and are on screen.

-- @agoose77

2024-08-21.17-18-17.mp4

Fixes #170

@agoose77 agoose77 changed the title Feat/outline collapse Make document outline collapsible May 10, 2024
@agoose77 agoose77 changed the title Make document outline collapsible 🔍 Make document outline collapsible May 10, 2024
@agoose77 agoose77 changed the base branch from main to agoose77/wip-outline-inline May 10, 2024 15:19
Base automatically changed from agoose77/wip-outline-inline to main May 21, 2024 15:41
@stevejpurves
Copy link
Contributor

👍🏼 cool - I like the use of radix-ui in here! I guess the next step for this is to hook it up to an intersection observer or something to auto-collapse on margin content?

@agoose77 agoose77 marked this pull request as draft July 5, 2024 13:42
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 6, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: f539088

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@myst-theme/site Patch
@myst-theme/article Patch
@myst-theme/book Patch
@myst-theme/providers Patch
@myst-theme/frontmatter Patch
@myst-theme/diagrams Patch
@myst-theme/jupyter Patch
@myst-theme/styles Patch
@myst-theme/common Patch
@myst-theme/icons Patch
myst-to-react Patch
myst-demo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@agoose77
Copy link
Collaborator

agoose77 commented Aug 6, 2024

I've done some general refactoring work here too, which touches a reasonable amount of this code. I'm growing in my comfort with React perf specifics, so I might have made a regression here. Would reviewers be able to try this out with that in mind?

@agoose77 agoose77 marked this pull request as ready for review August 6, 2024 14:11
agoose77
agoose77 reviewed Aug 6, 2024
View reviewed changes
packages/site/src/components/DocumentOutline.tsx
Comment on lines -65 to -69
getHeaders(selector).forEach((h) => {
h.classList.remove(HIGHLIGHT_CLASS);
});
el.classList.add(HIGHLIGHT_CLASS);
highlight?.();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have removed this in favour of letting the intersection observer handle it. Wouldn't be surprised if this was a specific bugfix -- I didn't check the blame.

agoose77
agoose77 reviewed Aug 6, 2024
View reviewed changes
themes/book/app/components/ArticlePage.tsx
@@ -75,7 +76,7 @@ export const ArticlePage = React.memo(function ({
const tree = copyNode(article.mdast);
const keywords = article.frontmatter?.keywords ?? [];
const parts = extractKnownParts(tree);

const isOutlineMargin = useMediaQuery('(min-width: 1024px)');
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice not to need this, but I think it's the cleanest approach.

@choldgraf
Copy link
Contributor

Could you add a GIF of what this looks like to the top comment? I can't figure out how to build previews locally and we don't have a PR-based preview available, so it's hard to know what the UX is like.

@agoose77
Copy link
Collaborator

@choldgraf done!

choldgraf
choldgraf approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@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 looks great to me from an end functionality perspective! As long as we aren't introducing any bugs or weird edge case regressions I'd be +1 on merging if somebody gives a +1 on the tech implementation

@agoose77 agoose77 mentioned this pull request Aug 28, 2024
Closed
@agoose77
Copy link
Collaborator

@stevejpurves I think the total set of class names that intersect with the margin is:

col-margin-right-inset
col-gutter-outset-right
col-screen-right
col-screen-inset-right
col-page-right
col-page-inset-right
col-body-outset-right
col-gutter-page-right

Based upon https://jupyter-book.github.io/myst-theme/?path=/docs/components-grid-system--docs. Do you agree?

@stevejpurves
Copy link
Contributor

@agoose77 if you don't include col-screen, col-page, col-page-inset and col-body-outset could, for example, a full width figure spanning out from the main body content use one of those or is it expected that col-page-inset-right, col-body-outset-right would cover those cases?

@rowanc1 rowanc1 merged commit ab19540 into main Sep 24, 2024
2 checks passed
@rowanc1 rowanc1 deleted the feat/outline-collapse branch September 24, 2024 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agoose77 agoose77 agoose77 left review comments

@choldgraf choldgraf choldgraf approved these changes

Assignees

@rowanc1 rowanc1

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hide TOC when it intersects with margin content
4 participants
@rowanc1 @stevejpurves @agoose77 @choldgraf