astropy / astropy-sphinx-theme

Standalone version of the Astropy sphinx theme
BSD 3-Clause "New" or "Revised" License
4 stars 15 forks source link

Update layout.html file in sphinx to add top menubar to astropy docs #4

Closed kakirastern closed 5 years ago

kakirastern commented 5 years ago

Addresses Issue astropy/astropy#8320. Am in the process of implementing changes to add top menubar to astropy docs. Might need help in debugging.

pllim commented 5 years ago

cc @astrofrog

kakirastern commented 5 years ago

Thanks, @pllim! Appreciate it

kakirastern commented 5 years ago

Hi! I just found out some issues with my PR and need more time to debug. But any suggestions or ideas would be appreciated.

astrofrog commented 5 years ago

Looking at the changes, I think most of these are specific to the core package, so I think we should customize the layout template there, not in this theme package (which is used by many packages)

bsipocz commented 5 years ago

@astrofrog - good point, it's my bad to forget about the packages that use the template but are not coordinated packages (for those this could be useful, too I think).

kakirastern commented 5 years ago

Yup @astrofrog, no problems! It is my first time so I did not check carefully enough before opening this PR, will effect the changes in my astropy/astropy repo then. Cheers!

kakirastern commented 5 years ago

Reopened this PR to follow up as suggested in https://github.com/astropy/astropy/pull/8440#pullrequestreview-209927291.

kakirastern commented 5 years ago

Hi @astrofrog, I have added an astropy-custom-menu as a html_theme option to the astropy-sphinx-theme repo in my PR. So now, at least in principal, if I set html_theme = 'astropy-custom-menu' instead of the default html_theme = 'bootstrap-astropy' option, the desired changes in the menubar should be effected. Please advise if this is what you had in mind as well.

kakirastern commented 5 years ago

And give me a brief moment to undo the previous commits regarding the bootstrap-astropy theme...

kakirastern commented 5 years ago

Think I have cleaned up every unnecessary change made in this PR prior to the last commit. Please review and confirm. Thanks!

kakirastern commented 5 years ago

And just to make sure, if I have changed the stylesheet in the theme.conf file, then I no longer need to add the new theme CSS style sheet to the layout.html file? Is my understanding of this correct?

kakirastern commented 5 years ago

I think I have managed to add the proposed menubar by configuring the theme at the astropy/astropy repo without having to resort to change the theme here. Therefore, I am closing this PR.

kakirastern commented 5 years ago

Hi, I have reopened this PR, and have added in items in the top menubar that would only be on if the sphinx variable astropy_project_menubar is set to True, so that people can opt out say by setting it to False. The PR is ready for a review now, as I was able to use a version of it on an affiliated repo to check locally, and obtained the desired effects. For a screenshot please see the following:

Screenshot 2019-03-15 22 43 02

kakirastern commented 5 years ago

You are welcome @astrofrog! Yes, the custom.css is supposed to replace bootstrap-astropy.css, that's why the significant overlapping between the two files. But I initiated wanted to check first before making the replacement final.

kakirastern commented 5 years ago

Replacement of bootstrap-astropy.css file completed.

astrofrog commented 5 years ago

In addition to the small comment above, can you also rebase this?

kakirastern commented 5 years ago

Yup, rebased and squashed all commits into one and resolved one conflict in the layout.html file.

astrofrog commented 5 years ago

@kakirastern - there was an issue with the rebase, and a residual merge commit, so I've rebased it again and pushed to your branch. I think this should then be good to go.

astrofrog commented 5 years ago

@kakirastern - the option couldn't currently be set from the conf.py file so I've fixed that up and also added a changelog entry and information in the README.

astrofrog commented 5 years ago

I've also removed the astropy_banner_96.png file which was incorrectly added to this PR.