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

📏 Allow full-length TOC when sidebar is useable #441

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented Jul 31, 2024

Presently, the TOC is always trimmed to the page length, in full-width OR narrow displays:

  • Wide:
    image
  • Narrow
    image

This PR tweaks the computation to detect when the "wide" media query is true/false:

  • Wide:
    image
  • Narrow:
    image

I think there might be some overflow problems, but I don't think they're caused by this PR.

Copy link

changeset-bot bot commented Jul 31, 2024

🦋 Changeset detected

Latest commit: d7adea6

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/providers Patch
@myst-theme/site Patch
@myst-theme/book 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-theme/article 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 Author

This PR bumps each package as a patch change. Is that right? Technically we're now exporting two new things from ui/providers.

@agoose77 agoose77 requested a review from rowanc1 July 31, 2024 15:36
@agoose77
Copy link
Collaborator Author

Note: I want to generalise the TOC sidebar into a primary sidebar, but that's more work.

Comment on lines 120 to 131
return (
<UiStateProvider>
<ArticlePageAndNavigationInternal
children={children}
hide_toc={hide_toc}
projectSlug={projectSlug}
inset={inset}
/>
</UiStateProvider>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a nicer way to access the result of a useContext inside the component tree?

Copy link
Contributor

Choose a reason for hiding this comment

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

do you mean that you want to access the UIStateProvider context within this same function, rather than creating a new wrapper component? you can do something like this but it's not nicer...

return (
    <UiStateProvider>
        {(() => {
            const uiState = useUiState();
            return (
                <ArticlePageAndNavigationInternal
                    children={children}
                    hide_toc={hide_toc}
                    projectSlug={projectSlug}
                    inset={inset}
                />);
            })
        }
    </UiStateProvider>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yep, that's one way!

@rowanc1
Copy link
Member

rowanc1 commented Jul 31, 2024

Where is the footer coming from in the UI?

@agoose77
Copy link
Collaborator Author

Where is the footer coming from in the UI?

Oh, haha, I appended it to the dom during testing. It only matters presently for the mystmd website.

@agoose77 agoose77 changed the title 📏 Allow full-width TOC when sidebar is useable 📏 Allow full-length TOC when sidebar is useable Aug 5, 2024
@rowanc1 rowanc1 requested a review from stevejpurves August 20, 2024 15:43
@rowanc1 rowanc1 assigned stevejpurves and agoose77 and unassigned stevejpurves Aug 20, 2024
@rowanc1 rowanc1 changed the title 📏 Allow full-length TOC when sidebar is useable 📏 Allow full-length TOC when sidebar is useable Aug 20, 2024
@agoose77
Copy link
Collaborator Author

This is now done!

@agoose77 agoose77 assigned stevejpurves and unassigned agoose77 Aug 23, 2024
@agoose77 agoose77 force-pushed the agoose77/fix-non-wide-sidebar branch from 208613d to 916620d Compare August 27, 2024 10:52
@agoose77
Copy link
Collaborator Author

@stevejpurves I rebased this on main, would you be able to confirm it still behaves as expected on mystmd.org?

@stevejpurves stevejpurves force-pushed the agoose77/fix-non-wide-sidebar branch from 4e8f9b0 to d7adea6 Compare September 5, 2024 20:48
@stevejpurves
Copy link
Contributor

rebased again, review in progress locally... having to update mystmd.org on some orthogonal changes that have happened whilst this PR has been alive...

@stevejpurves
Copy link
Contributor

all looks good, sorry for the lengthy review cycle!

@stevejpurves stevejpurves merged commit 3ae88e1 into main Sep 6, 2024
2 checks passed
@stevejpurves stevejpurves deleted the agoose77/fix-non-wide-sidebar branch September 6, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants