Skip to content

To add top menubar to Astropy docs webpages - #8440

Closed
kakirastern wants to merge 3 commits into
astropy:masterfrom
kakirastern:add-top-menubar-docs
Closed

kakirastern wants to merge 3 commits into
astropy:masterfrom
kakirastern:add-top-menubar-docs

Conversation

@kakirastern

@kakirastern kakirastern commented Feb 21, 2019 •

Copy link
Copy Markdown
Contributor

Fixes #8320.

To add new menubar items using the theme at the repo astropy/astropy-sphinx-theme as a backbone, then configure it to add the new top menubar for the Astropy docs webpages in the "conf.py" file in this repo, and add the relevant files needed such as a "custom.css" for styling the new menubar.

@kakirastern

Copy link
Copy Markdown
Contributor Author

The contents should show after astropy/astropy-sphinx-theme#4 has been merged successfully provided no bug present in the new code snippets inserted.

@pllim

pllim commented Feb 21, 2019

Copy link
Copy Markdown
Member

Unfortunately, over here, we don't do direct HTML. Rather, we use RST and have Sphinx render it to HTML. So I don't think this solution is correct?

@kakirastern

Copy link
Copy Markdown
Contributor Author

Will need to change files to RST then?

@kakirastern

Copy link
Copy Markdown
Contributor Author

And probably will need to modify the relative paths of the files links in the astropy sphinx theme as well. I was wondering about this myself earlier...

@pllim

pllim commented Feb 21, 2019

Copy link
Copy Markdown
Member

I think so, if you want Sphinx to render it.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Cool, let me get onto it tonight...

@kakirastern

Copy link
Copy Markdown
Contributor Author

Updated astropy-sphinx-theme "layout.html" file in commit bb801ee695b3124ee078d7a8d6daf762c2d53d4b of astropy/astropy-sphinx-theme#4 to correct relative paths to planned RST counterparts as replacements.

@codecov

codecov Bot commented Feb 21, 2019 •

Copy link
Copy Markdown

Codecov Report

Merging #8440 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8440   +/-   ##
=======================================
  Coverage   86.81%   86.81%           
=======================================
  Files         386      386           
  Lines       58098    58098           
  Branches     1060     1060           
=======================================
  Hits        50435    50435           
  Misses       7048     7048           
  Partials      615      615
Impacted Files Coverage Δ
astropy/io/fits/column.py 90.36% <0%> (-0.01%) ⬇️
astropy/coordinates/builtin_frames/__init__.py 100% <0%> (ø) ⬆️
astropy/units/equivalencies.py 96.19% <0%> (+0.01%) ⬆️

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 d9d1fe4...a1abb5f. Read the comment docs.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Just created my first RST file on GitHub with GitHub as "about.rst"... Felt really good about the process as it is much simpler than marking up using HTML/CSS + JavaScript. I still need to copy the image "Images/Numfocus_stamp.png" from the source repo to this repo for the sponsor logo to show though.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Argh... accidentally got rid of some commits while rebasing. Will try again.

@bsipocz

bsipocz commented Feb 21, 2019

Copy link
Copy Markdown
Member

@kakirastern - I think we need to find a way to only link to these pages in the website repo rather than having a copy of them here, too.
Having that said, I don't have a solution in mind of how to do it.

@bsipocz

bsipocz commented Feb 21, 2019

Copy link
Copy Markdown
Member

Wild guessing only: maybe we can build an intersphinx inventory for the website and use links from there in the theme? I'm not sure that would work or not, but maybe people involved in the learn site revamp will have a better idea.

@MananAgarwal @eteq @adrn

@kakirastern

Copy link
Copy Markdown
Contributor Author

@bsipocz Sure, no problem.

Also, should I follow the sphinx instructions from https://www.sphinx-doc.org/en/master/usage/extensions/intersphinx.html and follow it up from there? Will I need to update astropy/docs/conf.py accordingly? And, should I move the new RST files somewhere else first, or should I put them in a new folder in this repo and call it "menubar"? Let me know what you think.

@kakirastern

kakirastern commented Feb 22, 2019 •

Copy link
Copy Markdown
Contributor Author

If it is a repo contents organizational issue then I can move everything related to the menubar to a dedicated sub-folder/directory. That way is easiest to implement for Sphinx and it will be easier for other people to find the relevant documentation whenever changes need to be made. And, this is a safer way to make things work for the menubar, as I feel it is more foolproof to have everything related to the docs stay in the docs folder.

@kakirastern

Copy link
Copy Markdown
Contributor Author

@bsipocz Or a simpler approach that might work, which is to link interpreted text/phrase to external links generated in the webpage repo at astropy/astropy.github.com instead, so that it is guaranteed that the links I am trying to set up will only work if the homepage is not down, and the contents will be the same in all menubars (i.e. both on the landing page and in the docs).

For example, instead of rewriting a page called "about.rst", I just add an external link (rather than an internal one) to the corresponding "stable" page at http://www.astropy.org/about.html. That's probably the cleanest way to make things work.

This is perhaps a more desirable approach? Please let me know what you think.

@kakirastern

kakirastern commented Feb 23, 2019 •

Copy link
Copy Markdown
Contributor Author

I think I will need to add an inventory file indicated as None in "conf.py" in .txt or .inv format to the astropy/astropy.github.com repo to make it work, if this is indeed the right direction towards a functional solution...

@kakirastern

Copy link
Copy Markdown
Contributor Author

I have updated my PR at astropy/astropy-sphinx-theme#4 and now the menubar/navbar for the docs should work once that PR has been merged. Awaiting review though. Will leave the additional intersphinx_mapping commented out just in case. Can delete if desired.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Seems to be there was some problem with the unknown/nonexisting document "api/astropy.units.function.logarithmic.m_bol" when running the Circle CI linkcheck tests...

@kakirastern

Copy link
Copy Markdown
Contributor Author

Think the more important associated PR at astropy/astropy-sphinx-theme#4 as well as this PR are ready for a review. Any further comments, suggestions, or requests for changes are welcome.

@pllim

pllim commented Feb 25, 2019

Copy link
Copy Markdown
Member

@adrn , didn't you volunteer to be the webpage maintainer? Do you want to have a look? Thanks!

@kakirastern

kakirastern commented Feb 26, 2019 •

Copy link
Copy Markdown
Contributor Author

Got 10 warnings while building the html docs locally, as follows:

/Users/kastern/astropy/docs/nddata/index.rst:337: WARNING: Exception occurred in plotting index-4
 from /Users/kastern/astropy/docs/nddata/index.rst:
Traceback (most recent call last):
  File "/anaconda3/envs/astropy-dev/lib/python3.7/site-packages/matplotlib/sphinxext/plot_directive.py", line 499, in run_code
    exec(code, ns)
  File "<string>", line 20, in <module>
  File "/Users/kastern/astropy/build/lib.macosx-10.7-x86_64-3.7/astropy/utils/decorators.py", line 860, in block_reduce
    func = make_function_with_signature(func, name=name, **wrapped_args)
  File "/Users/kastern/astropy/build/lib.macosx-10.7-x86_64-3.7/astropy/nddata/decorators.py", line 245, in wrapper
    result = func(data, *args, **kwargs)
  File "/Users/kastern/astropy/build/lib.macosx-10.7-x86_64-3.7/astropy/nddata/utils.py", line 370, in block_reduce
    from skimage.measure import block_reduce
ModuleNotFoundError: No module named 'skimage'
/Users/kastern/astropy/docs/units/index.rst:386: WARNING: toctree references unknown document 'api/astropy.units.function.logarithmic.m_bol'
/Users/kasterni/astropy/docs/units/logarithmic_units.rst:241: WARNING: toctree references unknown document 'api/astropy.units.function.logarithmic.m_bol'

and

/Users/kastern/astropy/docs/index.rst:: WARNING: toctree contains reference to nonexisting document 'api/astropy.units.function.logarithmic.m_bol'
/Users/kastern/astropy/docs/index.rst:: WARNING: toctree contains reference to nonexisting document 'api/astropy.units.function.logarithmic.m_bol'
/Users/kastern/astropy/docs/units/index.rst:389:<autosummary>:1: WARNING: py:obj reference target not found: astropy.units.function.logarithmic.m_bol
/Users/kastern/astropy/docs/units/index.rst:: WARNING: toctree contains reference to nonexisting document 'api/astropy.units.function.logarithmic.m_bol'
/Users/kastern/astropy/docs/units/index.rst:389:<autosummary>:: WARNING: toctree contains reference to nonexisting document 'api/astropy.units.function.logarithmic.m_bol'
/Users/kastern/astropy/docs/units/logarithmic_units.rst:244:<autosummary>:1: WARNING: py:obj reference target not found: astropy.units.function.logarithmic.m_bol
/Users/kastern/astropy/docs/units/logarithmic_units.rst:244:<autosummary>:: WARNING: toctree contains reference to nonexisting document 'api/astropy.units.function.logarithmic.m_bol'

The majority of these warnings point towards the document 'api/astropy.units.function.logarithmic.m_bol'. Should I open another PR to deal with this?

@bsipocz

bsipocz commented Feb 26, 2019

Copy link
Copy Markdown
Member

@bsipocz

bsipocz commented Feb 26, 2019

Copy link
Copy Markdown
Member

The first issue should go away if you have scikit-image installed.

@kakirastern

Copy link
Copy Markdown
Contributor Author

@bsipocz Yup, have installed scikit-image and have run the tests again, now only 7 warnings related to the astropy/docs/units/index.rst file remains in running python setup.py build_docs. Again, all related to astropy.units.function.logarithmic.m_bol...

@kakirastern

Copy link
Copy Markdown
Contributor Author

Some Circle CI link check error persists...

@bsipocz

bsipocz commented Feb 27, 2019

Copy link
Copy Markdown
Member

It's the link checker, so we need to ignore it here.

@bsipocz

bsipocz commented Feb 27, 2019 •

Copy link
Copy Markdown
Member

@kakirastern - This basically waits for the theme PR, right? Otherwise does it work locally for you? It's a bit of hack to try to test it on CI (we rely on sphinx-astropy, that brings in the theme, so all of those need to change to point to the branch of your fork rather than the released versions). So overall it's easier to see whether everything works locally as they suppose than hack this system.

@kakirastern

Copy link
Copy Markdown
Contributor Author

PR opened to polish up top menubar at astropy/astropy-sphinx-theme#9. I have used some very scientific method to match the length of the elements observed with the pixels represented, so hopefully no further adjustments needs to be made. (Sorry about some troubles with this PR earlier as I feel like working while blindfolded as some coordination between repos is required.) Turned out according to RTD giles provides a better estimate of what the RTD rendering would look like so my instinct to follow the local build was baseless and misguided.

The issue now is two-fold:

  1. The dropdown menu height is too long (by around one fifth of current height)
  2. The spacing between topbar items is too tiny (such as the dropdown button for item "About" is closer to "Get Help" than it should)

Both have been addressed in the aforementioned PR.

@bsipocz bsipocz modified the milestones: v3.2.1, v3.2.2 Jun 15, 2019
@bsipocz bsipocz modified the milestones: v3.2.2, v4.0 Sep 27, 2019
@astropy-bot

astropy-bot Bot commented Sep 29, 2019

Copy link
Copy Markdown

Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock.

If you believe I commented on this pull request incorrectly, please report this here.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Should I open a new PR in lieu of this one to follow up?

@bsipocz

bsipocz commented Sep 29, 2019

Copy link
Copy Markdown
Member

No need, finishing this up would be the thing to do.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Then I think this PR is ready for a final review, as the related PR on the astropy-sphinx-theme repo has already been merged, at least that's my best understanding of the situation.

@bsipocz

bsipocz commented Oct 1, 2019

Copy link
Copy Markdown
Member

Based on the Giles rendering here, it doesn't seem exactly right for several reasons. I don't know how to fix these, but this cannot be merged until these issues are addressed.

  • the top bar is fully on the right side, it doesn't the layout at all of astropy.org

  • the dropdown floats away:

Screen Shot 2019-09-30 at 21 08 07

@kakirastern

Copy link
Copy Markdown
Contributor Author

Yeah, we did notice it is rendering differently on different platforms. (If my memory is any accurate it was rendering okay on @astrofrog's computer.) Okay, now I know how improve on it and make it work, I will submit more commits later this and next week to complete the PR.

@bsipocz

bsipocz commented Oct 1, 2019

Copy link
Copy Markdown
Member

basically the only platform that matter now is RTD. Not exactly sure how to test that easily.

@kakirastern

Copy link
Copy Markdown
Contributor Author

Yeah, true. I think I might be able to figure out a way around RTD rendering. Let me look into it more over the next two weeks.

@kakirastern13

Copy link
Copy Markdown

I checked and there is another PR opened by me in the astropy-sphinx-theme repo to deal with exactly this issue: astropy/astropy-sphinx-theme#9. Have been waiting for @astrofrog to have the time to work on the Astropy website again. Not sure when that will be however.

@astropy-bot

astropy-bot Bot commented Oct 31, 2019

Copy link
Copy Markdown

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

@astropy-bot astropy-bot Bot added the closed-by-bot Closed by stale bot label Oct 31, 2019
@astropy-bot astropy-bot Bot closed this Oct 31, 2019
@astrofrog astrofrog reopened this Oct 4, 2020
@pep8speaks

Copy link
Copy Markdown

Hello @kakirastern 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

Line 147:51: W291 trailing whitespace

@astrofrog

Copy link
Copy Markdown
Member

@kakirastern - could you rebase this so we can see the RTD preview here?

@kakirastern

Copy link
Copy Markdown
Contributor Author

@astrofrog Sure, will rebase soon

Kris Stern added 2 commits October 4, 2020 22:58
@pep8speaks

Copy link
Copy Markdown

Hello @kakirastern 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

Line 156:51: W291 trailing whitespace

@kakirastern

Copy link
Copy Markdown
Contributor Author

Think the css styling issue is awaiting to be addressed here: astropy/astropy-sphinx-theme#9

@astrofrog

Copy link
Copy Markdown
Member

@kakirastern - as discussed on Slack, could you simply get rid of the drop-down menu for now so we can get this in?

@kakirastern

Copy link
Copy Markdown
Contributor Author

Hey👋🏼 @astrofrog Was worried all tte items would not all fit into the nav bar neatly... Tried it before a long time ago

@eteq

eteq commented Dec 15, 2020

Copy link
Copy Markdown
Member

closed at request of author - last SHA 7f44951

@eteq eteq closed this Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add top menubar from astropy.org

7 participants