Skip to content

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

Merged
astrofrog merged 3 commits into
astropy:masterfrom
kakirastern:add-top-menu-bar-to-sphinx
Apr 24, 2019
Merged

astrofrog merged 3 commits into
astropy:masterfrom
kakirastern:add-top-menu-bar-to-sphinx

Conversation

@kakirastern

Copy link
Copy Markdown
Contributor

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

pllim commented Feb 27, 2019

Copy link
Copy Markdown
Member

cc @astrofrog

@kakirastern

Copy link
Copy Markdown
Contributor Author

Thanks, @pllim! Appreciate it

@bsipocz
bsipocz requested a review from astrofrog February 27, 2019 21:43
@kakirastern

Copy link
Copy Markdown
Contributor Author

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

astrofrog commented Feb 28, 2019 •

Copy link
Copy Markdown
Member

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

bsipocz commented Feb 28, 2019

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

Reopened this PR to follow up as suggested in astropy/astropy#8440 (review).

@kakirastern

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

@kakirastern

Copy link
Copy Markdown
Contributor Author

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

@kakirastern

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

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

@astrofrog astrofrog left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! This is on the right track, but at the moment the custom.css file duplicates a lot of what's in bootstrap-astropy.css - do we really need to have two separate CSS files? Either we should just have one file, or if we really need to override some CSS in the case where there is a toolbar, we can just override the styles from bootstrap-astropy.css. Ideally we'd just have one file.

@kakirastern

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor Author

Replacement of bootstrap-astropy.css file completed.

<link href='https://fonts.googleapis.com/css?family=Source+Sans+Pro:200,600' rel='stylesheet' type='text/css'/>
{% if astropy_project_menubar == true %}
<link type='text/css' href='_static/custom.css' />
{% endif %}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@kakirastern - is this needed anymore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yup, it is need, for people to opt in or out of the astropy_project_menubar as an option.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I confused this with something else. It is indeed not needed any more and has been removed.

@astrofrog

Copy link
Copy Markdown
Member

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

@kakirastern

Copy link
Copy Markdown
Contributor Author

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

@astrofrog

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

@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

Copy link
Copy Markdown
Member

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

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