-
Notifications
You must be signed in to change notification settings - Fork 325
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
i18n: Add full support #399
Conversation
This should work now. I'll test on my docs site. |
Okay, this is ready for review. It works! |
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.
Thanks for this! A few quick comments in there. Also a couple points that didn’t make sense for in line comments
- is there a natural way to test for this infrastructure? Do most packages include translation testing in their test suite? Ideally without complicating things much?
- We should add in a bit more explanation about how the translation infra works - I added a few suggestions in the review. In my experience people don’t know anything about Sphinx translation so this will help get oriented
Also feel like we should get sign off from @jorisvandenbossche to make sure he’s ok with this new infra setup
@@ -529,7 +532,7 @@ def setup(app): | |||
|
|||
# Update templates for sidebar | |||
pkgdir = os.path.abspath(os.path.dirname(__file__)) | |||
path_templates = os.path.join(pkgdir, "_templates") | |||
path_templates = os.path.join(pkgdir, "templates") |
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.
Flag for @jorisvandenbossche, any reason we wouldn’t want to use templates wo the underscore? I don’t think this would affect anything downstream
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.
It won't affect theme users. When theme users override a template, it's based on the file's basename, not on its path within a particular theme, to my knowledge.
docs/user_guide/i18n.rst
Outdated
|
||
This theme contains translatable strings. Most strings will automatically be translated if the language is `supported <https://github.com/pydata/pydata-sphinx-theme/tree/master/pydata_sphinx_theme/locale>`__. To add another language, see :ref:`translating-the-theme`. | ||
|
||
The theme's :doc:`options<configuring>` also contain translatable strings, which require a different process. The following instructions assume that you store your translations in a ``locale`` directory under your documentation directory, and that you want to use ``theme`` as the name the message catalog for these strings. |
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.
We should link to a useful introduction for Sphinx translation, in my experience most users don’t know how this works so I think this would helo
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.
I don't know that any are especially helpful. See e.g. https://www.sphinx-doc.org/en/master/usage/advanced/intl.html
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
I've updated |
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.
Thanks for those edits, this is looking really good. A few more relatively small comments.
Quick comment: fully on board with improving the theme in this area, assuming we follow the sphinx standard practices (and @jpmckinney thanks a lot for working on this!) |
…me as creating MO files
Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
Different projects do different things.
Alternately, you can decide to do the translations steps before a release only. |
Hey all - I think that this PR is an important improvement in the theme, and we don't want it to go unresolved indefinitely. Since we've already got a lot of iteration here, and since @jpmckinney has put a lot of time into this already. I suggest we go with the following steps:
@jpmckinney would you be willing to do no. 1? Sorry that this one has gone stale! |
Restarted at #1192. |
resolves #257