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

Replace vendored theme with commonjs import and bower sources #1645

Merged
merged 9 commits into from
Sep 15, 2015

Conversation

agjohnson
Copy link
Contributor

This stops using manually vendorized theme bits in several places:

  • Vendorized theme templates
  • Theme javascript copypasta
  • Manually imported CSS files for the theme
  • Manually imported font files

This complete drops the vendorized theme 🔥

I don't know what purpose it served in the past, but it seems unnecessary when
we're installing the theme in each virtualnev anyways. I could have missed
something here. This leaves the layout.html override, but removes the other
files which all looked unused or unwarranted.

Theme javascript copypasta is replaced with a CJS import from a bower-installed
sphinx-rtd-theme package. The manually imported CSS and font files from the theme are
also installed from bower sources now. Unnecessary links for font files were dropped based on hit in our server logs.

Bower/pip are temporarily pointing at a branch on the theme repo, this will be
changed to a release or release tag after.

Blocked by readthedocs/sphinx_rtd_theme#243
Fixes #1501

Gulp will copy these files into our vendored sources from now on,
installing from the sphinx_rtd_theme github repo.
I looked at one of the web logs hits to these files for this::

   3750 /font/fontawesome_webfont.eot
     34 /font/fontawesome_webfont.svg
    788 /font/fontawesome_webfont.ttf
  79785 /font/fontawesome_webfont.woff
   4897 /fonts/fontawesome-webfont.eot?v=4.2.0
     60 /fonts/fontawesome-webfont.svg?v=4.2.0
   1280 /fonts/fontawesome-webfont.ttf?v=4.2.0
 130268 /fonts/fontawesome-webfont.woff?v=4.2.0
    153 /fonts/Inconsolata-Bold.ttf
    175 /fonts/Inconsolata.ttf
 127495 /fonts/Lato-Bold.ttf
 136011 /fonts/Lato-Regular.ttf
 129953 /fonts/RobotoSlab-Bold.ttf
 102762 /fonts/RobotoSlab-Regular.ttf
@gregmuellegger
Copy link
Contributor

Local test was successful. Pop-out works, theme is cool. No 404 requests from the docs page.
I only don't understand what you are doing with the fonts, however it seems to work :)

Everything else looks good to me.

@agjohnson agjohnson removed the Status: blocked Issue is blocked on another issue label Sep 15, 2015
@agjohnson
Copy link
Contributor Author

This just moves the theme font/css bundling to bower + gulp along with newer assets. It doesn't necessarily need to be from bower sources, but it at least seems a little more predictable, or at least explicit.

agjohnson added a commit that referenced this pull request Sep 15, 2015
Replace vendored theme with commonjs import and bower sources
@agjohnson agjohnson merged commit 0b78fcd into master Sep 15, 2015
html_theme_path.append('{{ template_path }}')
else:
html_theme_path = ['_themes', '{{ template_path }}']

Copy link

Choose a reason for hiding this comment

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

I think this broke the build for us: https://readthedocs.org/projects/lasagne/builds/
Before this PR it worked, now it says "ThemeError: no theme named 'sphinx_rtd_theme' found (missing theme.conf?)"
The relevant section in our conf.py is as follows:

if os.environ.get('READTHEDOCS') != 'True':
    try:
        import sphinx_rtd_theme
    except ImportError:
        pass  # assume we have sphinx >= 1.3
    else:
        html_theme_path = [sphinx_rtd_theme.get_html_theme_path()]
html_theme = 'sphinx_rtd_theme'

That is, if running outside of readthedocs and with an old version of Sphinx, it imports sphinx_rtd_theme and sets html_theme_path. Then it sets html_theme, no matter if running on readthedocs or not. This worked fine, now it doesn't.

Note that we have Sphinx==1.2.3 in our requirements.txt because of sphinx-doc/sphinx#1822 (which will only be fixed in Sphinx 1.4). We then relied on readthedocs setting the theme path for us.

Why has this been removed, and how can we work around it? (I assume if we just remove the os.environ.get('READTHEDOCS') check, we'd fall back to using the PyPI version of sphinx-rtd-theme. But I'd rather like to hear your suggestion than committing a couple of different things to try!)

Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is still an issue, open a ticket, it will have more visibility. I believe we deployed a fix for this yesterday.

Copy link

Choose a reason for hiding this comment

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

Thanks for getting back to me! I was aware that this was not the most obvious place to report problems, but figured you'd be notified and you'd know best. The build works again if we intend the html_theme = line so the variable is not set when running on readthedocs. Cheers!

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.

3 participants