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

Improving ABlog style and configuration #1075

Merged
merged 3 commits into from
Dec 6, 2022
Merged

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Dec 4, 2022

This PR improves a number of the ABlog style and layout, and fixes up some of our configuration setting to set config values at the right point of the Sphinx build.

Here are the major changes:

  • Fixing CSS rules for ABlog that were broken when we change the HTML sidebar layout (this will be fixed more long-term when we fix Update ABlog CSS now that it has better-labeled HTML #1067 (but that is waiting on an ABlog release)
  • Improves the spacing and styling for more ABlog metadata
  • Enables FontAwesome by default in ABlog
  • Moves our update_config function earlier in the Sphinx build event chain, I think to where we are "supposed" to be updating configuration values.
  • Makes our ABlog docs use slightly more complex link structure so we test out this functionality better

Demo here 👉 https://pydata-sphinx-theme--1075.org.readthedocs.build/en/1075/user_guide/ablog.html

@choldgraf
Copy link
Collaborator Author

cc for @12rambau - I couldn't get the new djlint pre-commit hook to work. This is the error I got:

An error has occurred: InvalidManifestError: 
==> File /home/choldgraf/.cache/pre-commit/repo6qrk7oys/.pre-commit-hooks.yaml
==> At Hook(id='djlint-nunjucks')
==> At key: types_or
==> At index 0
=====> Type tag 'nunjucks' is not recognized.  Try upgrading identify and pre-commit?

Did you run into this at all?

@12rambau
Copy link
Collaborator

12rambau commented Dec 5, 2022

Did you run into this at all?

The way it displays error is not always straight forward (specifically the first time when nothing was prettiffied but no I never saw this one

I'm going to try on my side

@12rambau
Copy link
Collaborator

12rambau commented Dec 5, 2022

yep I confirmed, nothing from my side:

MacBook-Pro-de-pierrick-rambaud:pydata-choldagraf pierrickrambaud$ git checkout --track origin/enh-pydata
la branche 'enh-pydata' est paramétrée pour suivre 'origin/enh-pydata'.
Basculement sur la nouvelle branche 'enh-pydata'
MacBook-Pro-de-pierrick-rambaud:pydata-choldagraf pierrickrambaud$ pre-commit run -a
[INFO] Initializing environment for https://github.com/PyCQA/flake8.
[INFO] Installing environment for https://github.com/PyCQA/flake8.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
prettier.................................................................Passed
black....................................................................Passed
flake8...................................................................Passed
check builtin type constructor use.......................................Passed
check for case conflicts.................................................Passed
check toml...............................................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
pyupgrade................................................................Passed
djLint for Jinja.........................................................Passed

@choldgraf
Copy link
Collaborator Author

OK I pushed a commit that adds a test for this so that we can confirm our "config override" behavior! I think this is ready to go unless folks have style suggestions

@@ -1044,7 +1048,7 @@ def setup(app):
app.set_translator("readthedocs", BootstrapHTML5Translator, override=True)
app.set_translator("readthedocsdirhtml", BootstrapHTML5Translator, override=True)

app.connect("env-updated", update_config)
app.connect("builder-inited", update_config)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the fact that you now intercept "builder-inited" instead of "env-update" means that you take into account default values from theme.conf.

Does it means that verything that is done in prepare_html_config could be moved to update_config ?

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 do think that this is the "cleaner" way to intercept and/or set our own defaults for config. Happy to explore moving that functionality to this event if you think that'd be useful

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would make sense in a folow-up PR but great that you found out this event !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A quick follow-up here. I actually don't think that we can move the theme-related deprecations into the builder-inited event.

This is because builder-inited (by its name) comes after the HTML builder has been inited, and at that point the theme's configuration structure has already been finalized. We can then alter that structure later on in the html-page-context event, but if we alter it in app.config earlier on, it will have no effect.

@choldgraf choldgraf self-assigned this Dec 6, 2022
@12rambau 12rambau merged commit def2139 into pydata:main Dec 6, 2022
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.

Update ABlog CSS now that it has better-labeled HTML
2 participants