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

To add top menubar to Astropy docs webpages #8440

Closed
wants to merge 3 commits into from
Closed

To add top menubar to Astropy docs webpages #8440

wants to merge 3 commits into from

Conversation

kakirastern
Copy link
Contributor

@kakirastern kakirastern commented Feb 21, 2019

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
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
Copy link
Member

pllim commented Feb 21, 2019

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
Contributor Author

Will need to change files to RST then?

@kakirastern
Copy link
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
Copy link
Member

pllim commented Feb 21, 2019

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

@kakirastern
Copy link
Contributor Author

Cool, let me get onto it tonight...

@kakirastern
Copy link
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
Copy link

codecov bot commented Feb 21, 2019

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
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
Contributor Author

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

@bsipocz
Copy link
Member

bsipocz commented Feb 21, 2019

@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
Copy link
Member

bsipocz commented Feb 21, 2019

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
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
Copy link
Contributor Author

kakirastern commented Feb 22, 2019

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
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
Copy link
Contributor Author

kakirastern commented Feb 23, 2019

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
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
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
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
Copy link
Member

pllim commented Feb 25, 2019

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

@kakirastern
Copy link
Contributor Author

kakirastern commented Feb 26, 2019

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
Copy link
Member

bsipocz commented Feb 26, 2019

Weird, I'm not sure why that page is not generating as it should, it the latest docs everything is still nominal:
http://docs.astropy.org/en/stable/api/astropy.units.function.logarithmic.m_bol.html?highlight=m_bol

@bsipocz
Copy link
Member

bsipocz commented Feb 26, 2019

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

@kakirastern
Copy link
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
Contributor Author

Some Circle CI link check error persists...

@bsipocz
Copy link
Member

bsipocz commented Feb 27, 2019

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

@bsipocz
Copy link
Member

bsipocz commented Feb 27, 2019

@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.

@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
Copy link

astropy-bot bot commented Sep 29, 2019

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
Contributor Author

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

@bsipocz
Copy link
Member

bsipocz commented Sep 29, 2019

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

@kakirastern
Copy link
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
Copy link
Member

bsipocz commented Oct 1, 2019

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
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
Copy link
Member

bsipocz commented Oct 1, 2019

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

@kakirastern
Copy link
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

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
Copy link

astropy-bot bot commented Oct 31, 2019

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 closed this Oct 31, 2019
@astrofrog astrofrog reopened this Oct 4, 2020
@pep8speaks
Copy link

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
Member

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

@kakirastern
Copy link
Contributor Author

@astrofrog Sure, will rebase soon

@pep8speaks
Copy link

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
Contributor Author

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

@astrofrog
Copy link
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
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
Copy link
Member

eteq commented Dec 15, 2020

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