-
Notifications
You must be signed in to change notification settings - Fork 35
👣 Allow custom primary sidebar footer #613
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
Conversation
🦋 Changeset detectedLatest commit: 804f922 The changes in this PR will be included in the next version bump. This PR includes changesets to release 15 packages
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 |
|
I definitely agree we need to let users disable this if they wish. I have one suggestion but it would require a bit of extra work so it's not a huge deal if you dont have the bandwidth for it now: Rather than making this a feature flag, could we make it an option to modify the content in the sidebar footer? For example rather than |
|
@choldgraf that's a neat rendition of this issue! I'll expand this PR to feature your suggestion too. |
site.option.hide_myst_branding option site.option.primary_sidebar_footer option
|
@choldgraf this works on my end — I've added a sidebar_footer option to |
|
Ah interesting - so you can define the sidebar footer in a separate part rather than in the config directly? Could you give demo config or a link to an example that I could use to try out? |
site.option.primary_sidebar_footer option site.parts.sidebar_footer option
|
@choldgraf updated the PR description to include an example! Overall, this feels like a better approach than writing a footer in MyST Markdown in the config. |
|
Thanks for authoring this PR @artoftheblue! I saw your request for review on Discord. I need to commit more time than I have today to review this PR, because there are a few things we need to think about such as context-providers. I'm away next week, so hopefully someone else can pick up the reviewing role before then! |
|
@agoose77 sure! Thanks for the heads-up. |
|
Just noting that a similar thing was added here for the site-wide footer: My assumption would be that this can follow a similar pattern. Perhaps @stefanv could provide guidance about how that was implemented and how this is similar or different? |
Yes, I think this PR already uses part in the same way, so 👍 It could be useful to wrap in a special element or a div, so it can be styled in CSS. See https://github.com/jupyter-book/myst-theme/blob/main/themes/book/app/components/Footer.tsx#L8. |
|
Would be good to also add some documentation for this feature. |
|
@choldgraf see also! |
✅ Deploy Preview for myst-theme ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
site.parts.sidebar_footer option | } | ||
|
|
||
| return ( | ||
| <div className="text-sm text-slate-900 dark:text-white [&>*]:m-0 [&_a]:underline [&_a]:font-semibold"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a myst- class here to enable styling.
Link this to #667 so that we can follow up there.
|
I'm not totally sure yet on whether we should add special styling to sidebars, etc. I don't think we do for footer, on the basis it can have wierd consequences for complex markup trees. |
|
@agoose77 So, what would you like to see changed here before merge? |
|
I'm I terested in what others think. We can probably ship something and refine it down the road if needs be. |
|
My feeling is that this is a simple change that unblocks users who don't want the made with myst logo forced on them, or who otherwise want a custom sidebar footer. I'm not sure what you mean by "special styling", but I am fine adding any language to these docs to clarify that anything "special" is not supported, even going so far as to say only common mark is supported. It is easy to add more functionality in the future if need be, and I think we can bias towards incremental progress by merging this so long as there isn't a major strategic risk here. My criteria for major strategic risks would be:
My suggestion here (and in every other PR) is that if concerns don't rise to this level, we should just merge things and move forward. For an under-resourced project like ours, slowing down too much is a bigger risk than moving too quickly. |
Co-authored-by: Chris Holdgraf <[email protected]>
| sidebarRef={toc} | ||
| hide_toc={hide_toc} | ||
| footer={<MadeWithMyst />} | ||
| footer={<SidebarFooter content={projectParts?.primary_sidebar_footer?.mdast} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow-up, we may want to rename footer here to something more descriptive, since this does not refer to the main page footer.
Maybe sidebarFooter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, ideally this would be primarySidebarFooter and it could live alongside secondarySidebarFooter one day. Or, call it something more generic like primarySidebarEnd to recognize that other things than footers might be placed there one day.
I think my suggestion would be we just try to re-use the JB1 structure naming as much as possible for the book theme, since it has been used and battle-tested for a long time now:
https://pydata-sphinx-theme.readthedocs.io/en/stable/user_guide/layout.html

Originally was supposed to implement functionality discussed in
Made with MySTplaque below the TOC toggleable mystmd#2080However, the PR was expanded to allow the user to define a custom footer in the primary sidebar to replace the default
Made with MySTfooter.The config is similar to how the site-wide footer is currently added:
For example, creating the file named
sidebar_footer.mdwith the following contentsrenders as