-
-
Notifications
You must be signed in to change notification settings - Fork 740
WIP: Add beeware.org header to Furo Sphinx theme. #3538
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
base: main
Are you sure you want to change the base?
Conversation
docs/_static/custom.css
Outdated
/* For continuity, we may want to enable the following: */ | ||
/*.page h1, .page h2, .page h3, .page h4, .page h5, .page h6 {*/ | ||
/* font-family: 'Cutive', serif;*/ | ||
/*}*/ | ||
|
||
/*.sidebar-brand-text {*/ | ||
/* font-family: 'Cutive', serif;*/ | ||
/*}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want the docs to have a similar feel to the beeware.org website, it may be prudent to add this in, as well as the more detailed header-bits from beeware.css
.
Oof. The spelling check on |
Acknowledged - but that seems manageable. We can do a preview whenever the furo theme updates and confirm any issues.
This definitely appeals as a stylistic consistency tweak. If we can make it work technically, it seems worth it. IIRC most of the header extras are to deal with the fact that the hinting on the Cutive font is... weird (as in - the fonts declared bounding box isn't the extents of the characters), and so additional padding is needed to compensate without the page looking overly compressed in places.
Ack - I'm sure we'll find other tweaks and edge cases as we go, but as a first pass, what you've got here seems well inside the bounds of "acceptable shenanigans", and looks great.
This was my immediate thought as well. If we can't link directly to the Bootstrap CDN (and I can't remember why I/we did that, other than not trusting the CDN at some point), we should be able to rely on serving the content
One can never have too many bees. 🤣 But yes, I'd be surprised if we need all of those - The only usage should be favicons, plus the icon in the header bar; it's entirely possibly you can get away with just the -32 variant. |
Gah - posted that early - I wanted to add an additional overall comment: Overall - this looks great. There's some minor tweaks and tuning to be done (as you've acknowledged), but I think this is a great visual consistency improvement. Other than the general tweaks and things you've already flagged, my only other comment is that we need to find a way to provide a link to the GitHub page in the sidebar somewhere (possibly 2 links - one for "source", and one for "issues"). |
Thank you. I'm quite pleased with it.
This may have been the source of my CSS nightmare in getting this to work. Regardless, I grabbed the entirety of the header settings from
This hadn't occurred to me. It is now pulling from Superfluous bees also removed.
This has been added. CSS can obviously be tweaked as desired. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've flagged a couple of things inline that stood out from a quick inspection, but otherwise - this is definitely on the right track.
enough with Sphinx that you know what you're doing, or landed here from that | ||
documentation page. | ||
|
||
Hope your day's going well. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤣
Is this your doing, or Pradyuns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Entirely Pradyun's. It exists in all the files that you can customise through the Furo config, as an attempt to avoid you unnecessarily doing what we're doing. It is necessary for us, so here we are. Probably not worth removing, as any future diff would include it and show up as a change to be updated. That said, I can obviously remove it if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh no - keep it in. If someone is elbow deep in this file, they deserve a little pick me up :-)
<div class="collapse navbar-collapse" id="navbarsDefault"> | ||
<ul class="navbar-nav mr-auto"> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="https://beeware.org/about">About</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Observation - these render on beeware.org as "unselected" unless it's the current page. I'm pretty sure we can just render projects
(or Docs, if that's where it ends up) as permanently selected, and all others as unselected.
</button> | ||
<div class="nav-top"> | ||
<div class="navbar-brand-block"> | ||
<a class="navbar-brand" href="https://beeware.org"><img class="mx-2" src="/_static/images/brutus-32.png">BeeWare</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't rendering on the RTD preview... I suspect because the root URL for RTD won't have this. Don't know if there's a better fix for this, and maybe we just have to live with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other problem with troubleshooting this is that it renders in testing. So I would have to start making changes, pushing them, and seeing whether RtD shapes up. I'm ok with doing this, but not unless you confirm you want me to. Please let me know your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK with this being a "known issue" with testing. We already have analogous issues with font-awesome icons; as long as we know the limitation exists, we can work around it.
I was more hoping there might be a "{{ root_url }}" template piece that could be used - but if there isn't, that's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this was a thing. It renders locally with the latest addition. We'll see if it works with RtD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - I have no idea if {{root_url}}
actually exists... it may be working locally because "undefined variable" renders as "" in Jinja.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh fair enough. I'll dig a bit further...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So RtD evidently automatically supplies a html_baseurl
, which can also be manually set in conf.py
. I can't find any reasonable information about actually using it in HTML or Jinja. If you have any suggestions, I'm happy to try them. I tried {{ html_baseurl }}
but, as you said, it'll work locally if it is undefined. So I didn't bother pushing that change.
{# Custom JS #} | ||
{%- block regular_scripts -%} | ||
<script src="https://code.jquery.com/jquery-3.2.1.slim.min.js" integrity="sha384-KJ3o2DKtIkvYIK3UENzmM7KCkRr/rE9/Qpg6aAZGJwFDMVNA/GpGFF93hXpG5KkN" crossorigin="anonymous"></script> | ||
<script src="https://beeware.org/static/bootstrap/js/bootstrap.min.js"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the look of the javascript console when running locally,=, there's another JS file needed here (popper.js)
{%- endblock -%} | ||
|
||
<!-- Font Awesome --> | ||
<script src="https://kit.fontawesome.com/078c86a30a.js" crossorigin="anonymous"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mental note that I may need to modify my font-awesome domain permission here.
docs/_static/custom.css
Outdated
.github-links { | ||
font-family: 'Cutive', serif; | ||
padding: var(--sidebar-item-spacing-vertical) var(--sidebar-item-spacing-horizontal); | ||
font-size: 1.25em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure we need this level of size emphasis; the link existing (maybe with a font-awesome Github icon as well?) is probably enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rest of the ToC tree is set to be smaller than the standard unset font size. I realised after the fact, that these two screenshots look really similar. I promise they are different.
Do you want it left to its own devices, which is shown in the first screenshot?
Or would you prefer I make it the same size as the ToC tree, shown in the second screenshot? It uses a variable to do this, the reuse of which seems to be working successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Further, two questions:
- Do you want it Cutive or do you want it to match the ToC tree font?
- I tried to add a Font Awesome icon, and I'm apparently unclear on doing so. Is this related to the permissions thing you mention in another comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with ToC tree sizing, but we might as well use Cutive to make it stand out a bit.
As for the font-awesome icon... I've modified the config so that RTD builds and toga.readthedocs.io
should have permission to load. Locally, localhost
is allowed to load... but 127.0.0.1
isn't, apparently.
docs/_static/custom.css
Outdated
} | ||
|
||
.main { | ||
margin-top: 1em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is leading to a weird gap with the toc drawer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm uncertain why I included this. Removing.
docs/_static/custom.css
Outdated
} | ||
|
||
.sidebar-drawer, .toc-drawer { | ||
padding-top: 3em; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also contributing to the toc drawer spacing issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to compensate for the header height on smaller displays. I updated it to apply only in the scenario when it is necessary.
This reverts commit aefa04f.
The BeeWare documentation pages are not connected visually with the beeware.org website. In preparation for a major restructure of the documentation, it was suggested that perhaps we could implement the beeware.org header on the docs pages. This is an initial proof of concept.
This implements the HTML injection feature in the Furo Sphinx theme, along with updates to the Sphinx configuration to enable it. I initially had the full
beeware.css
file from the website, but as there was already CSS present incustom.css
, I grabbed my updates from thebeeware
file and added them tocustom
.Potential issues:
base.html
if the Furo theme updates it. The sites will not stop working, but any updated features would not be present.I have some initial questions/comments.
beeware.css
file, I think.beeware.css
file as well. This is a proof of concept. We can optimise if we choose to continue down this path.PR Checklist: