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

upgrade to bootstrap / bootswatch v3.3.7 #164

Merged
merged 3 commits into from
Jul 23, 2017
Merged

upgrade to bootstrap / bootswatch v3.3.7 #164

merged 3 commits into from
Jul 23, 2017

Conversation

svenevs
Copy link
Collaborator

@svenevs svenevs commented Jul 21, 2017

  • minor doc fixes
    • I believe I got all 3.3.6 into 3.3.7
    • suggestion to use bs3 over 2
  • more specific directions on how to upgrade in future
    • missing documentation of static/bootstrap-sphinx.css_t for upgrades
    • much more description of what should go down in TODO.rst
      • modified the epic tar packaging to be more explicit (anybody who has egrep aliased can't use the original solution)
  • fixed Makefile to actually use python3
    • added simple option for port override (I'm currently running a mission critical app on 8000 xD)
  • fixed requirements.txt to allow e.g. sphinx==1.6.2 or higher
    • setup.py has neither of these dependencies?
    • if you add them, you'll need to treat python 2 and 3 separately because of fabric?

Notes:

  • Candidate to close Support for bootstrap/bootswatch 3.3.7? #149
  • Officially the solar theme is not in the v3.3.7 bootswatch tag, see this issue
    • Just added it manually from the main page
  • I don't think you actually want to include the custom theme, I'm pretty sure it's either an artifact or a basis for creating your own.
  • Changed a few locations referring to the now nonexistent amelia theme

Questions:

  1. There is a rather long commented out section in TODO.rst. Might be worth deleting?
  2. See changes to README.rst, in particular the added .. warning::. The question: I didn't really understand what was going on with Switch to css imports rather than css_files hackery. #159 because I didn't have any trouble building with sphinx==1.5.1 or 1.5.5 etc. Basically, if it's actually a hard requirement then maybe the verbiage is fine, but if not then it's too strong?
  3. This is not a "you" problem, but when viewing the source of generated pages I noticed all of the javascript gets loaded at the beginning in the <meta> rather than at the bottom. I know next to nothing about front-end, but those are supposed to be loaded last right (sphinx is doing this, not you)?

Happy to undo / revise any of the included, thanks again for the theme 😄

- minor doc fixes
- more specific directions on how to upgrade in future
Copy link
Owner

@ryan-roemer ryan-roemer left a comment

Choose a reason for hiding this comment

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

Tremendous PR @svenevs !!!

Great documentation and instructions for future upgrades. The demo looks good to me when I've pulled this down.

The only items I'd like to see changed are:

  1. I've put a comment about compatibility with sphinx <= 1.5 re the CSS stuff. Can you check that your documentation changes are accurate because I do think we still support sphinx <= 1.5
  2. I've got some local changes that I'd like to see go in this PR. To make sure I don't forget, can you apply the following diff too?
diff --git a/.gitignore b/.gitignore
index b2e04df..4f9d226 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,9 +1,11 @@
-\.DS_Store
-*\.pyc
+.DS_Store
+*.pyc
 
 build
 dist
-*\.egg-info
+*.egg-info
 demo/build/*
 node_modules
 __pycache__
+.vscode
+
diff --git a/demo/source/conf.py b/demo/source/conf.py
index cf06dd0..99c9883 100644
--- a/demo/source/conf.py
+++ b/demo/source/conf.py
@@ -140,7 +140,7 @@ html_theme_options = {
     # Bootswatch (http://bootswatch.com/) theme.
     #
     # Options are nothing (default) or the name of a valid theme such
-    # as "amelia" or "cosmo".
+    # such as "cosmo" or "sandstone".
     #
     # Example themes:
     # * flatly
diff --git a/requirements.txt b/requirements.txt
index 306aff5..a8342eb 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -1,2 +1,2 @@
-Fabric==1.6.0
+Fabric>=1.13.2
 Sphinx>=1.6.1

README.rst Outdated

Add a `setup` function in "conf.py" with stylesheet paths added relative to the
The Sphinx Bootstrap Theme does not provide legacy Sphinx support, the
Copy link
Owner

Choose a reason for hiding this comment

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

Is this correct?

My understanding is that the new @import CSS sheets put into place after #159 works for both sphinx <= 1.5 and sphinx > 1.5. It's just the "how do I override" instructions that differ...

@svenevs
Copy link
Collaborator Author

svenevs commented Jul 22, 2017

Ok I added the diff you wanted in there, made the demo_server target depend on demo so it builds everything, and changed the verbiage a little bit with respect to sphinx versions.

Yes, I agree I don't think there are any incompatibilities with respect to older sphinx versions. However, the reason I am confused is because app.add_stylesheet has been around for a long time, if you search for add_stylesheet in the changelog you will see it surface as early as sphinx==1.0, where the comment of "supports full URIs" for version 1.1 seems to be the important part -- anything after should accept app.add_stylesheet without issue.

AKA I'm kind of leaning toward the original issue not being entirely accurate? When building locally on say sphinx==1.5.5, I had no issues with app.add_stylesheet, but I might be missing the bigger picture.

@ryan-roemer
Copy link
Owner

@svenevs -- Changes look great. I'm going to merge it and release.

Re: the add_stylesheet, the situation evolved the other way -- I wasn't even aware that was an option until finding out the old hacky way of set css_files = stopped working. A PR to collapse everything into "just use add_stylesheet" and not split on Sphinx versions would definitely be welcome!

@ryan-roemer ryan-roemer merged commit f7c86aa into ryan-roemer:master Jul 23, 2017
@ryan-roemer
Copy link
Owner

@ryan-roemer
Copy link
Owner

@svenevs -- I don't usually do this on a first PR, but I'm making you a contributor to this project. Your changes here to not only the core project, but the meta-instructions as well have been fabulous. If you've got free time and desire, I'd love to any maintenance help / review that you wish to do. If not, thanks for all the hard work in this PR!

In the future, just open branches instead of fork PRs, as I can more easily jump in to push minor changes.

Feel free to DM me on twitter https://twitter.com/ryan_roemer if you want to chat out any additional details.

@pcav
Copy link

pcav commented Jul 24, 2017

Thanks! Upgraded seamlessly on our website: http://www.faunalia.eu

@svenevs svenevs deleted the upgrade_v3.3.7 branch July 27, 2017 02:18
@svenevs
Copy link
Collaborator Author

svenevs commented Jul 27, 2017

@ryan-roemer Woot! Hehehe yes I kind of obsess over documenting documentation systems for some reason. My plate's more of an overflowing bowl right now, but I do have some thoughts / things I'd like to incorporate in the (hopefully not too distant) future.

@pcav great!!! Thanks for giving feedback that it all works as expected 😄

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.

Support for bootstrap/bootswatch 3.3.7?
3 participants