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

Run grunt #522

Merged
merged 7 commits into from Dec 28, 2017

Conversation

Projects
None yet
3 participants
@Blendify
Contributor

Blendify commented Dec 21, 2017

  • Update grunt file with changes done to docs
  • Update grunt sass to 1.0.0 to avoid warning message

Blendify added some commits Dec 21, 2017

Run Grunt
Also fix some errors from recent docs rename
Upgade grunt contrib sass to 1.0.0
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.
```

@Blendify Blendify requested review from ericholscher, agjohnson and jessetan Dec 21, 2017

@jessetan

LGTM

Show outdated Hide outdated Gruntfile.js

Blendify added some commits Dec 22, 2017

@agjohnson

The Gruntfile changes look fine, but there are some extra changes that are included here that I think block this PR.

@@ -16,17 +16,22 @@ function ThemeNav () {
isRunning: false
};
nav.enable = function () {
nav.enable = function (withStickyNav) {

This comment has been minimized.

@agjohnson

agjohnson Dec 24, 2017

Collaborator

What is the purpose of this addition? If configurable, this needs more information or documentation.

@agjohnson

agjohnson Dec 24, 2017

Collaborator

What is the purpose of this addition? If configurable, this needs more information or documentation.

This comment has been minimized.

@jessetan

jessetan Dec 27, 2017

Contributor

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?

@jessetan

jessetan Dec 27, 2017

Contributor

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?

This comment has been minimized.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

Yeah, not important here I think, but having our public API documented would make sense for folks extending or repurposing the theme.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

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>");

This comment has been minimized.

@agjohnson

agjohnson Dec 24, 2017

Collaborator

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.

@agjohnson

agjohnson Dec 24, 2017

Collaborator

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.

This comment has been minimized.

@jessetan

jessetan Dec 27, 2017

Contributor

This comes from #488, see comments there.

@jessetan

jessetan Dec 27, 2017

Contributor

This comes from #488, see comments there.

This comment has been minimized.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

I've opened #530 to experiment with removing this pattern.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

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 };

This comment has been minimized.

@agjohnson

agjohnson Dec 24, 2017

Collaborator

This likely breaks on RTD. Has this been tested?

@agjohnson

agjohnson Dec 24, 2017

Collaborator

This likely breaks on RTD. Has this been tested?

This comment has been minimized.

@jessetan

jessetan Dec 27, 2017

Contributor

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.

@jessetan

jessetan Dec 27, 2017

Contributor

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.

This comment has been minimized.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

At some point we were overriding or referencing StickyNav in RTD. So this would be worth testing.

@agjohnson

agjohnson Dec 27, 2017

Collaborator

At some point we were overriding or referencing StickyNav in RTD. So this would be worth testing.

@Blendify

This comment has been minimized.

Show comment
Hide comment
@Blendify

Blendify Dec 24, 2017

Contributor

All the changes done to theme.js were made in separate PRs run git blame to see the changes.

Contributor

Blendify commented Dec 24, 2017

All the changes done to theme.js were made in separate PRs run git blame to see the changes.

@jessetan

This comment has been minimized.

Show comment
Hide comment
@jessetan

jessetan Dec 27, 2017

Contributor

This is a good example of confusion caused by the built JS being included in PRs, as discussed in #379

Contributor

jessetan commented Dec 27, 2017

This is a good example of confusion caused by the built JS being included in PRs, as discussed in #379

@agjohnson

This comment has been minimized.

Show comment
Hide comment
@agjohnson

agjohnson Dec 27, 2017

Collaborator

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.

Collaborator

agjohnson commented Dec 27, 2017

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.

@Blendify Blendify merged commit 19b22eb into master Dec 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jessetan jessetan deleted the rungrunt branch Dec 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment