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

Update the display for sidebar navigation to include section names #3910

Merged
merged 7 commits into from
Mar 6, 2019

Conversation

sunithabeeram
Copy link
Contributor

@sunithabeeram sunithabeeram commented Mar 5, 2019

Update the sidebar display as shown in the screenshot. Main change required was to update index.rst to have the :caption: keyword. However, with the referenced sphinx theme, the display of highlevel sections (for ex, "Introduction" in the sidebar) was in a light color (not quite readable). Following the instructions on https://sphinx-rtd-theme.readthedocs.io/en/latest/installing.html, I downloaded the theme and included it in the docs folder and that has made it work better.

screenshot from 2019-03-05 15-03-00

Copy link
Contributor

@mcvsubbu mcvsubbu left a comment

Choose a reason for hiding this comment

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

dont know how to review this, but if you can pls create an issue that "View on Gitgub button" is missing, we can add that at a later date.

@sunithabeeram
Copy link
Contributor Author

@mcvsubbu it looks like the local build isn't displaying the Edit on Github link. Will create an issue if it still is an issue on the main rtd link.

@snleee
Copy link
Contributor

snleee commented Mar 5, 2019

As we discussed offline, we should do the following:

  1. remove license check for _themes
  2. remove apache license header for external code (css, html, js..etc) and keep them the same as the original code.
  3. update LICENSE file

@sunithabeeram
Copy link
Contributor Author

@snleee updated.

Copy link
Contributor

@snleee snleee left a comment

Choose a reason for hiding this comment

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

It seems that copying sphinx_rtd_theme pulls another transitive dependencies of some other external js/css/font files. Those need to be handled separately for LICENSE. I think that we can deal with this before cutting a next apache release. For now, let's add sphinx_rtd_theme to LICENSE file.

We can also consider to keep only image & rst files within the source code and come up with some build script that downloads the required js/css/html files (or themes). That will be much cleaner and simpler.

LICENSE Outdated
@@ -218,6 +218,7 @@ pinot-controller/src/main/resources/*/js/lib/handlebars.js
pinot-controller/src/main/resources/*/css/lib/codemirror*
pinot-controller/src/main/resources/*/css/lib/foundation*
pinot-controller/src/main/resources/*/css/lib/normalize.css
docs/_themes/*
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it has to be more specific docs/_themes/sphinx_rtd_theme/* and put its license as LICENSE-sphinx_rtd_theme.txt since sphinx project is different from sphinx_rtd_theme project

@sunithabeeram
Copy link
Contributor Author

sunithabeeram commented Mar 6, 2019

@snleee I've removed the includes for now based on our discussion. Locally, the sphinx-build version I have is v1.6.3. Its perhaps an older one and its possible that the one thats on rtd is newer (I also saw other differences). Will push this code and build it out to ensure things work correctly.

@sunithabeeram sunithabeeram merged commit 867edcf into master Mar 6, 2019
@sunithabeeram sunithabeeram deleted the docs branch March 6, 2019 01:00
@sunithabeeram
Copy link
Contributor Author

readthedocs build uses sphinx v1.7.9 and that renders the captions correctly.

@codecov-io
Copy link

Codecov Report

Merging #3910 into master will decrease coverage by 25.71%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master    #3910       +/-   ##
=============================================
- Coverage     67.13%   41.41%   -25.72%     
=============================================
  Files          1032     1155      +123     
  Lines         50884    57812     +6928     
  Branches       7108     8015      +907     
=============================================
- Hits          34161    23943    -10218     
- Misses        14391    31837    +17446     
+ Partials       2332     2032      -300
Impacted Files Coverage Δ Complexity Δ
...inot/core/data/readers/GenericRowRecordReader.java 0% <0%> (-100%) 0% <0%> (ø)
.../org/apache/pinot/common/http/MultiGetRequest.java 0% <0%> (-100%) 0% <0%> (ø)
...n/function/PercentileEstMVAggregationFunction.java 0% <0%> (-100%) 0% <0%> (ø)
...che/pinot/common/restlet/resources/TablesList.java 0% <0%> (-100%) 0% <0%> (ø)
...che/pinot/pql/parsers/pql2/ast/OptionsAstNode.java 0% <0%> (-100%) 0% <0%> (ø)
...ers/pql2/ast/PredicateParenthesisGroupAstNode.java 0% <0%> (-100%) 0% <0%> (ø)
...ot/core/query/scheduler/TableBasedGroupMapper.java 0% <0%> (-100%) 0% <0%> (ø)
...t/core/segment/index/readers/OnHeapDictionary.java 0% <0%> (-100%) 0% <0%> (ø)
...ommon/utils/retry/RetriableOperationException.java 0% <0%> (-100%) 0% <0%> (ø)
...t/core/query/scheduler/OutOfCapacityException.java 0% <0%> (-100%) 0% <0%> (ø)
... and 641 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7a8bb6...1976eaf. Read the comment docs.

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.

4 participants