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

Custom extensions #45

Merged
merged 25 commits into from Mar 20, 2022
Merged

Custom extensions #45

merged 25 commits into from Mar 20, 2022

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Mar 18, 2022

New Directives

These new directives are automatically added to doc builds because they are tied to our CSS:

  • md-tab-set and md-tab-item which resolves Content tabs #43 (includes a new doc page to demonstrate CSS class/id usage)
  • md-mermaid to expose the implementation merged in from upstream (includes a new doc page to demonstrate CSS class/id usage).

Patches

Known bug (very minor annoyance)

Upon page refresh, the tabs' highlighted bottom border always resets to the first tab (unexpected behavior), but the displayed tab content and highlighted tab label do not reset (expected behavior). I suspect it has something to do with how the JS loads on page refresh, but I'm not too concerned at this point.

before refresh

image

after refresh

image

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 18, 2022

I've started seeing a deprecation warning when executing python setup.py bdist_wheel cmd:

/path/to/sphinx-immaterial/setup.py:21: DeprecationWarning: The distutils package is deprecated and slated for removal in Python 3.12. Use setuptools or check PEP 632 for potential alternatives
  import distutils.command.build

Seems to only appear in python v3.10+ or could be caused by pip v22.0.4 - CI is currently using python v3.9.x and pip v22.0.3


EDIT: This warning is resolved by putting

import setuptools  # pylint: disable=wrong-import-order

before the import to distutils.

docs/content_tabs.rst Outdated Show resolved Hide resolved
docs/content_tabs.rst Outdated Show resolved Hide resolved
docs/content_tabs.rst Outdated Show resolved Hide resolved
docs/content_tabs.rst Outdated Show resolved Hide resolved
setup.py Show resolved Hide resolved
@jbms
Copy link
Owner

jbms commented Mar 18, 2022

Overall, great work here!

I think we should try to fix the indicator line beneath the tab. Probably adding some console.log statements to the javascript code may help in figuring out what is going on.

@jbms
Copy link
Owner

jbms commented Mar 18, 2022

I looked into the indicator line issue -- turns out it is a bug in mkdocs-material which appears to have been fixed in the insiders version.

I reported it here:
squidfunk/mkdocs-material#3744

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Thanks for making the changes --- it is true there are many ways to implement things and it is not always clear what the best approach is --- I think it is true that your initial approach of using a post transform was perfectly valid. I just was recently trying to improve the efficiency of documentation generation for tensorstore, where due to the large number of pages generated for the api documentation, it can be rather slow, so I was acutely aware of efficiency concerns. My thinking is also that ideally the docutils nodes represent semantically meaningful things, and then the HTML translator converts that to HTML. In this case, the input elements aren't really a semantically meaningful thing on their own, they are just how it happens to be implemented in HTML, so it seems nice to encapsulate that in the HTML translator if possible.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Regarding the indicator line issue -- if that doesn't get fixed in upstream mkdocs-material soon that should be easy to fix here.

I also think it would be fairly easy to implement the linked tabs feature if we want it, though I wonder if that should be enabled via a :linked: option or similar, rather than a global "feature" option.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

I also think it would be fairly easy to implement the linked tabs feature if we want it, though I wonder if that should be enabled via a :linked: option or similar, rather than a global "feature" option.

Whatever upstream does to do it would be preferable. If it ends up being harder to implement here, then I would also favor a :linked: option as that also slightly resembles how sphinx-design does it.

  • Rather than use the label, the sphinx-design uses a :key: linked_label_x that is assign to each tab-item. This is of course more long-winded, but using RST is more flexible than MD in adding a specific flag to a block.

I wouldn't be surprised if we could support both approaches (global & tab-item keys) - not simultaneously I imagine.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

It wouldn't be hard to support it exactly as in mkdocs-material --- logically though it seems like it may be more useful to allow the user more flexibility. As you note --- it is easier to support additional options in rst than in markdown.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Thanks!

@jbms jbms merged commit f925a44 into jbms:main Mar 20, 2022
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

Either way, we'd have to brush up on rxjs syntax if the :linked: approach is added in with upstream's global implementation (in the same TS file). If I'm being honest, I feel like you're better equipped to make that kind of change. 😉

@2bndy5 2bndy5 deleted the custom-extensions branch March 20, 2022 01:48
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

I still don't fully understand pushing tags directly. I tagged the merge commit f925a44 with v0.4.0, but the CI didn't seem to run on that commit.

Docs are also not updated because the CI didn't run on main branch.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Yeah I can take a look at adding the linked support.

I am bit confused by what is going on with github actions --- we have seen issues before with it not triggering on this repository. The config is identical to what I have on other repositories where it seems to work reliably, so I'm not sure what is going on here.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Actually I'm not seeing your tag. After you create a tag locally, you have to specifically push the tags to the remote repository:

git push --tags origin

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

Its got to be related to PRs coming from other forks. I just switched my remotes to only use this repo from here on out. They recently added a new event (for security concerns) specific to this situation.

You can also use gh CLI to manually trigger a workflow.

RTD updated the "stable" build on the merging of this PR, so I think its related to event.

Actually I'm not seeing your tag. After you create a tag locally, you have to specifically push the tags to the remote repository:

I did, but then I removed it when I realized the CI wasn't triggered for main.


I'm going to try adding a workflow_dispatch to the CI's list of triggered events. then tag it with v0.4.0 again.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

well I got it to publish, but it keeps triggering twice

  1. for commit
  2. for the tag

This might be problematic for some scenarios of bumped version numbers. As witnessed when I pushed v0.3.1 tag:

Checking dist/sphinx_immaterial-0.3.post1.dev2-py3-none-any.whl: PASSED, with warnings
  warning: `long_description_content_type` missing. defaulting to `text/x-rst`.
Uploading distributions to https://test.pypi.org/legacy/
Uploading sphinx_immaterial-0.3.post1.dev2-py3-none-any.whl

  0%|          | 0.00/4.54M [00:00<?, ?B/s]
 85%|████████▌ | 3.86M/4.54M [00:00<00:00, 14.8MB/s]
100%|██████████| 4.54M/4.54M [00:01<00:00, 4.19MB/s]
HTTPError: 400 Bad Request from https://test.pypi.org/legacy/
Error during upload. Retry with the --verbose option for more details.
File already exists. See https://test.pypi.org/help/#file-name-reuse for more information.

@jbms
Copy link
Owner

jbms commented Mar 20, 2022

Yeah, it will trigger twice, but I think that isn't too big of a deal. The error you are seeing is for an upload to the test PyPI instance. Only the action run triggered by the tag push will upload to the non-test PyPI instance.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Mar 20, 2022

Unfortunately, such an error will cause the image badge on the readme to indicate the failure to upload to test-pypi. I believe we can tell the workflow to ignore the error for that specific step

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants