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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge upstream v8.5.6 #171

Merged
merged 13 commits into from Oct 24, 2022
Merged

Merge upstream v8.5.6 #171

merged 13 commits into from Oct 24, 2022

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Oct 20, 2022

patch_info.zip

This wasn't as painful as I had expected (due to the long time since last update from upstream), or maybe I'm just getting better at it (wishful thinking 馃ぃ )

Aside from the usual JS tweaks after the merge script did it's thing, I also

I had to tweak the merge script for files that are explicitly excluded (in MKDOCS_EXCLUDE_PATTERNS) and are no longer in the upstream base/target git indexes. There's still room for improvement because I had to manually add the new files from upstream since they weren't in this repo's index. The additions were mostly language translations and a couple features implemented on upstream only (like builtin site feedback and comments). Maybe there's a way to port the new functionality to sphinx builds, but I'm not overly enthusiastic about them (currently content with directing to other sites like github for a more dynamic user interactive experience).

I did discover 1 regression about mermaid graphs and filed an issue upstream (squidfunk/mkdocs-material#4517). We can merge in the simple fix for that later though.

also updates the setup-graphviz action, but I'm not sure this will
get rid of the deprecation warning in the workflow runs about
using "Node.js 12 actions"
enables linked content tabs and explains/demos how to use them in docs
@2bndy5 2bndy5 linked an issue Oct 20, 2022 that may be closed by this pull request
@2bndy5 2bndy5 mentioned this pull request Oct 20, 2022
docs/content_tabs.rst Outdated Show resolved Hide resolved
{document.tags && document.tags.map(tag => (
<span class="md-tag">{tag}</span>
))}
{document.tags && (
Copy link
Owner

Choose a reason for hiding this comment

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

I guess we could see about supporting tags?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm far from an expert on the search mods. Now that I think about it, that is something I didn't check before pushing.

Copy link
Owner

Choose a reason for hiding this comment

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

supporting the tags in search would be more work. But there is also a mechanism to just display the tags on the page that would be simpler to support --- we would just need a way to specify them as part of the file metadata. Maybe without search integration they aren't very useful though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm leaving this as is for now. If someone asks, then we can discuss it again (or just create a new issue as a reminder).

@jbms
Copy link
Owner

jbms commented Oct 20, 2022

Thanks for doing this upgrade!

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 20, 2022

If you don't mind I'd like to keep this open for testing and further needed documentation about newer features. I'm about to push changes to docs about customizing the admonition icons. I also intend to patch the hide-feedback and hide-footer meta tags.

@jbms
Copy link
Owner

jbms commented Oct 20, 2022

If you don't mind I'd like to keep this open for testing and further needed documentation about newer features. I'm about to push changes to docs about customizing the admonition icons. I also intend to patch the hide-feedback and hide-footer meta tags.

Sure, that's why I didn't merge it myself.

The metadata keys using hyphens was apparently broken; I'm not sure when
it was broke.

So, this fixes the `:hide-*:` metadata keys by using an
underscore instead: `:hide_*:`.
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 21, 2022

I'm now ready to merge this. I'll let it stew for a day so you have another chance to review the documentation changes (namely in customization.rst and admonitions.rst).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 21, 2022

The consent feature does work, but it's designed to not trigger on local files viewed in the browser.

I'm not sure how to document the cookies portion of the feature. The JS side seems obvious to me for custom cookies, but I'm not sure the upstream default cookies are enacted.

Here's the config I used to test it:

html_theme_options = {
    "consent": {
        "title": "Welcome!",
        "description": "We don't use cookies, so your privacy is safe here.",
        "actions": ["accept"],
    }
 }

and then manually remove the hidden attr in html because I was viewing it from a file.

I think I'll just add the necessary src code to use it and leave the documentation for another time.

includes external links to RST doctree specs
fix indent in example snippet
approved workaround to sphinx-doc/sphinx#10932 is to
use a blank line at start of doc before metadata
@jbms
Copy link
Owner

jbms commented Oct 24, 2022

Thanks, looks good to me.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 24, 2022

Great, I'll merge this and start/continue squashing open issues. I suppose we could tag this merger, but I'm unreasonably excited about code annotations (despite the implementation's current limits), and want to solve #169 before the next PyPI release.

@2bndy5 2bndy5 merged commit eedce14 into main Oct 24, 2022
@2bndy5 2bndy5 deleted the merge-upstream-v8.5.6 branch October 24, 2022 23:08
@@ -661,14 +676,18 @@

// Show navigation link as title
> .md-nav__link {
position: sticky;
top: 0;
z-index: 1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This z-index caused a regression as reported in #189

2bndy5 added a commit that referenced this pull request Nov 10, 2022
2bndy5 added a commit that referenced this pull request Nov 11, 2022
fixes #189 by using the inline CSS variable with fallback value inherited from upstream
2bndy5 added a commit that referenced this pull request Nov 15, 2022
fixes #189 by using the inline CSS variable with fallback value inherited from upstream
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.

Linked content tabs
2 participants