Run grunt #522
Run grunt #522
Conversation
Blendify
commented
Dec 21, 2017
- Update grunt file with changes done to docs
- Update grunt sass to 1.0.0 to avoid warning message
Fixes the following warning: ``` DEPRECATION WARNING: Passing --sourcemap without a value is deprecated. Sourcemaps are now generated by default, so this flag has no effect. ```
Gruntfile.js
Outdated
| @@ -133,17 +133,17 @@ module.exports = function(grunt) { | |||
| }, | |||
| /* Changes in theme dir rebuild sphinx */ | |||
| sphinx: { | |||
| files: ['sphinx_rtd_theme/**/*', 'demo_docs/**/*.rst', 'demo_docs/**/*.py'], | |||
| files: ['sphinx_rtd_theme/**/*', 'docs/**/*.rst', 'docs/**/*.py'], | |||
There was a problem hiding this comment.
Since README is now also built, README.rst should be added to wacthed files.
There was a problem hiding this comment.
Can we just check the full docs file? docs/**/* wont this catch everything?
There was a problem hiding this comment.
README.rst is not in the docs dir, it's in the root.
| @@ -16,17 +16,22 @@ function ThemeNav () { | |||
| isRunning: false | |||
| }; | |||
|
|
|||
| nav.enable = function () { | |||
| nav.enable = function (withStickyNav) { | |||
There was a problem hiding this comment.
What is the purpose of this addition? If configurable, this needs more information or documentation.
There was a problem hiding this comment.
This comes from #519. enable() was only called to enable sticky nav, while it was needed to decorate the navbar with the correct toggle options.
The extra bool was added so that a single entry point could be used to start the navigation logic (with or without sticky nav).
Documenting (with JSDoc?) of this file is a good idea, perhaps something for a separate issue?
There was a problem hiding this comment.
Yeah, not important here I think, but having our public API documented would make sense for folks extending or repurposing the theme.
| $("table.docutils.footnote") | ||
| .wrap("<div class='wy-table-responsive footnote'></div>"); | ||
| $("table.docutils.citation") | ||
| .wrap("<div class='wy-table-responsive citation'></div>"); |
There was a problem hiding this comment.
These changes are also unrelated to the PR. Is there background on these changes anywhere?
This pattern always seemed strange, especially when we have full control of the Sphinx writer as a Sphinx extension. The most correct is to do this on writing, not on each view I think. It might be best to split this work out into a separate PR if it isn't already.
There was a problem hiding this comment.
This comes from #488, see comments there.
There was a problem hiding this comment.
I've opened #530 to experiment with removing this pattern.
| @@ -170,7 +187,7 @@ function ThemeNav () { | |||
| module.exports.ThemeNav = ThemeNav(); | |||
|
|
|||
| if (typeof(window) != 'undefined') { | |||
| window.SphinxRtdTheme = { StickyNav: module.exports.ThemeNav }; | |||
| window.SphinxRtdTheme = { Navigation: module.exports.ThemeNav }; | |||
There was a problem hiding this comment.
This likely breaks on RTD. Has this been tested?
There was a problem hiding this comment.
This also comes from #519. The theme code that calls this has been changed accordingly.
It was tested on a local build, but I don't know if it has been tested on RTD. I can't imagine this function being called from outside of the theme though. A search in the RTD source does not turn up anything.
There was a problem hiding this comment.
At some point we were overriding or referencing StickyNav in RTD. So this would be worth testing.
|
All the changes done to |
|
This is a good example of confusion caused by the built JS being included in PRs, as discussed in #379 |
|
Agreed. Separating asset generation from feature PRs is a great help. In the interim, until we get more serious about our release process, separating asset generation into a changeset and noting the changeset in PR would make review slightly easier. We do something similar with linting review on RTD, but it's still difficult to review. Changes here make more sense now, think this can just be merged. |