-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Actual mobile toc; dark mode; speed up site loading #3950
Conversation
…side collapsible and mobile-friendly
…t customising toc display
…nt flash; optimise mobile menu display
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 file (as with header.html
) is designed to be shared and updated across the multiple sites that make up raspberrypi.com (e.g. Raspberry Pi Events, PIP, Raspberry Pi ID, the homepage, the news pages, etc.).
If you make changes here, they'll be lost whenever we need to update it for changes to the header or footer or are you wanting to break away from the site-wide versions and have a documentation-only version?
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.
To support dark mode, we do need to tweak the header and footer files, as otherwise they'll remain in light mode at all times. I've kept the tweaks minimal (mostly colour based, with some padding trimmed from the header to reduce excess space usage), so we could try to mostly share. But unless we add dark mode to the main site as well, this will require deviation.
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.
Given that this PR is modifying both the navigation and also adding a dark-mode, I wonder if it might be worth splitting that into two separate PRs? I.e. get the navigation changes merged in, and then add dark-mode theme as separate (later) step? 🤔 Or would it be far too much work to disentangle the two things?
Also, we'll probably want @jackbenwillis to design us some "light mode" and "dark mode" icons which would integrate with the design better than the generic sun and moon icons that you're currently using?
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.
Rather than modifying header.html
and footer.html
directly and thereby preventing us from easily updating them across the sites, would a middle ground be for you to add your own separate CSS overrides in your own stylesheet until we can upstream dark mode support in the header across the whole site?
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.
Great point. Opened https://github.com/raspberrypi/raspberrypi.com-header-and-footer/pull/81 to address this -- once we've merged that, I'll update this PR to use that new approach (and likely some new header and footer-specific variables).
@@ -1,5 +1,6 @@ | |||
<meta charset="utf-8"> | |||
<meta http-equiv="X-UA-Compatible" content="IE=edge"> | |||
<link rel="shortcut icon" href="{{ site.baseurl }}/favicon.ico"> |
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 different to the default behaviour of using https://www.raspberrypi.com/favicon.ico?
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.
Currently, local builds of the docs yell at you on the CLI about a missing favicon and lack a favicon entirely. I added this to fix the issue locally, and I think it shouldn't break anything. Though your sanity check is appreciated. Hopefully that makes sense now.
@@ -0,0 +1,3 @@ | |||
<div id="theme-toggle-container"> | |||
<a id="theme-toggle" alt="toggle site theme" title="toggle site theme" onclick="toggleTheme()"></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.
Would it be better to be unobtrusive here and add a click
event listener on the element (identifying it by its ID of theme-toggle
)?
e.g.
document.getElementById("theme-toggle").addEventListener("click", (e) => {
e.preventDefault();
toggleTheme();
});
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.
My understanding is that this ultimately does the exact same thing, but with an element lookup. Is there a performance benefit that I'm not aware of, or some penalty to using onclick?
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.
See my other comment re separating the HTML from the JavaScript but also the broader concept of unobtrusive JavaScript to encourage progressive enhancement.
{% include subtitle.html %} | ||
{% endif %} | ||
{% include tabs.html %} | ||
<body onload="makeToc()"> |
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.
As above re being unobtrusive: could you fire makeToc()
on DOMContentLoaded
?
document.addEventListener("DOMContentLoaded", () => {
makeToc();
});
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.
Similar question as the toggle comment above -- happy to move the call, but I'd like to know if there's a performance or organisational benefit before I do that!
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 can extract all your JavaScript into a separate file then it can be cached across page loads and served from our CDN while the HTML is typically always loaded from the origin servers.
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.
Ah, gotcha. I can pull the makeToc and theme stuff into a separate file, but the inline toc manipulation will have to stay here as it's part of the static site generation process.
jekyll-assets/_layouts/docs.html
Outdated
<a class="level3" href="{{ site.baseurl }}{{ item.path }}#{{ entry.anchor }}" > | ||
<a id="{{ item.path }}#{{entry.anchor}}-toc" class="toc-item" alt="expand documentation category" title="expand documentation category" | ||
> | ||
<style onload="if (window.location.hash.includes('{{ entry.anchor }}')) { document.getElementById('{{ item.path }}#{{entry.anchor}}-toc').setAttribute('style', 'font-weight:bold'); document.getElementById('{{ item.path }}#{{ entry.anchor }}').checked = true; }"></style> |
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.
Does this bit of JS belong only here or could it be extracted elsewhere given the associated style
element is empty?
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.
Good point; I can actually change this to a script
tag and remove the onload
call. We need to combine jekyll's liquid tags with javascript, so this call most likely needs to be in this file somewhere.
jekyll-assets/_layouts/docs.html
Outdated
onclick="document.getElementById('mobile-toggle').checked = false;" | ||
class="toc-item no-dropdown" | ||
> | ||
<style onload="if (window.location.hash.includes('{{ entry.anchor }}')) { document.getElementById('{{ item.path }}#{{entry.anchor}}-toc').setAttribute('style', 'font-weight:bold'); document.getElementById('{{ item.path }}#{{ entry.anchor }}').checked = true; }"> </style> |
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.
As above re using an empty style
tag purely to execute JavaScript.
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.
as above, so below!
…up some other files
I had a play with this on Nate's laptop this afternoon - it looks gorgeous! 😍 |
@@ -17,8 +17,9 @@ title: Raspberry Pi Documentation | |||
description: >- # this means to ignore newlines until "baseurl:" | |||
Raspberry Pi Documentation. | |||
baseurl: "/documentation" # the subpath of your site, e.g. /blog | |||
url: "" # the base hostname & protocol for your site, e.g. http://example.com |
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 sure that there must have been some reason why this was left empty; but that reason is probably now lost to the mists of time...
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 it might have an impact on the URLs used for the rptl header. Need to double-check the behaviour on a staging build before I release!
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 header uses all absolute URLs so it shouldn’t be affected.
@@ -1,5 +0,0 @@ | |||
<div class="contribute"> |
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.
Interestingly, it looks like "contribute" used to only be at the bottom of pages like https://www.raspberrypi.com/documentation/accessories/ but not at the bottom of pages like https://www.raspberrypi.com/documentation/accessories/sd-cards.html
I can't remember why! 😉
$ git grep -C4 footer.html
jekyll-assets/_layouts/boxes.html- </div>
jekyll-assets/_layouts/boxes.html-
jekyll-assets/_layouts/boxes.html- {% include contribute.html %}
jekyll-assets/_layouts/boxes.html- {% include copyright.html %}
jekyll-assets/_layouts/boxes.html: {% include footer.html %}
jekyll-assets/_layouts/boxes.html- {% include search.html %}
jekyll-assets/_layouts/boxes.html-
jekyll-assets/_layouts/boxes.html- </body>
jekyll-assets/_layouts/boxes.html-</html>
--
jekyll-assets/_layouts/docs.html- </section>
jekyll-assets/_layouts/docs.html- </div>
jekyll-assets/_layouts/docs.html-
jekyll-assets/_layouts/docs.html- {% include copyright.html %}
jekyll-assets/_layouts/docs.html: {% include footer.html %}
jekyll-assets/_layouts/docs.html- {% include scripts.html %}
jekyll-assets/_layouts/docs.html- {% include search.html %}
jekyll-assets/_layouts/docs.html-
jekyll-assets/_layouts/docs.html- </body>
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.
Weird. Because in the old system, only pages that have a GitHub link lack the contribute disclaimer.
Perhaps we should just get rid of it entirely. I suspect we should instead toss that text into the README.md or CONTRIBUTING.md for this repo (if we need it at all?)
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.
🤷 It's entirely up to you...
jekyll-assets/_includes/footer.html
Outdated
} | ||
#__rptl-footer .__rptl-footer-link, #__rptl-footer .__rptl-footer-link--with-icon { | ||
font-weight: 400; | ||
color: #333; | ||
color: var(--subtle); |
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 it correct that you've got both var(--subtle-text);
and var(--subtle);
?
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.
One is for subtle text. The other is for subtle UI elements :)
I'll unleash a round of comments and CSS organisation today to note some of my trials and errors for the maintainers of the future.
--subtle: #707070; | ||
--less-subtle: #333; | ||
--home: #cd2355; | ||
--rp2040-context-tag: #50C878; |
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.
Does there also need to be a --rp2350-context-tag
? 🤷
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'll take a look at the SDK doc today to check, great point. There probably SHOULD be!
jekyll-assets/css/style.css
Outdated
--less-subtle: #333; | ||
--home: #cd2355; | ||
--rp2040-context-tag: #50C878; | ||
--accent: #cd2355; |
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 know nothing about how all this works, but I see that you have:
--brand-colour: #cd2355;
--home: #cd2355;
--accent: #cd2355;
Is that intentional?
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.
Yep! I'll check to see if I can consolidate any of these, but in some cases those variables are different between the light and dark themes. In other cases, they have different semantic meaning. I should probably look through the HTML and make sure that I'm using them in a consistent way (and if I'm not, I should just use one variable).
--near-bg: #f6f6f6; | ||
--red-tint: #d75a64; | ||
--top-title-colour: #343434; | ||
--brand-colour: #cd2355; |
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.
Please forgive me if this is a stupid question, but if this --brand-colour
value is identical in both the :root
and .light
sections, does it really need to be specified in two separate places? (and obviously also any other colours that are the "same" in both sections)
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 a stupid question. Actually a stupid implementation from me. Part of my aforementioned consolidation of variables will be a 'common' non-theme-dependent set of vars.
jekyll-assets/scripts/theme.js
Outdated
function toggleTheme() { | ||
// fetch the theme from local storage (if it exists) | ||
var theme = localStorage.getItem('theme'); | ||
// if theme them has never been set, or is light, set the theme to the dark symbol in local storage to change 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.
"theme them" ?
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.
lol nice catch. Will fix.
There's a big picture question here whether the documentation site wants to keep the standard raspberrypi.com header and footer such that clicking between the homepage, news, and documentation sites retains an identical header at all browser sizes (especially as documentation is hosted in a subdirectory rather than its own subdomain) or whether it wants its own look and feel. I would prefer we stick with the standard header and instead upstream any changes you want to accommodate in your new design. Even though that is more effort, it would stop us being inconsistent across www.raspberrypi.com. Given the amount of changes here, as @lurch suggests, perhaps it is worth separating the cross-site changes from the documentation-specific ones? |
I can respect that we want a consistent header across the sites. We should chat about how the shared header can support dark mode on the docs site -- maybe you can use CSS variables for colours, explicitly set those variables on each of the subdirectories, and docs can handle the overrides (as I do now) for dark and light? |
I've added you to the repository for the raspberrypi.com header and footer if you want to look at adding CSS variables with fallbacks for colours you want to override. |
…tent to space out footer
… make sidebar cover footer on left side
…licate anchor names; improve search bar appearance on mobile; colour tweaks
…vious; mark tabs js as async so that it runs once tab elements exist
@@ -17,8 +17,9 @@ title: Raspberry Pi Documentation | |||
description: >- # this means to ignore newlines until "baseurl:" | |||
Raspberry Pi Documentation. | |||
baseurl: "/documentation" # the subpath of your site, e.g. /blog | |||
url: "" # the base hostname & protocol for your site, e.g. http://example.com |
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 header uses all absolute URLs so it shouldn’t be affected.
localStorage.setItem('theme', '🌝'); | ||
} | ||
// finally, toggle the light option off or on the body to change the display of the theme | ||
document.body.classList.toggle('light'); |
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.
Did you try setting color-scheme: light
(and color-scheme: dark
) and then relying solely on the prefers-color-scheme
media query? If that works, I wonder if it’d make it simpler to implement dark mode support in the header and footer without relying on specific class names.
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.
Unfortunately a lot of browsers don't support the prefers-color-scheme query for anti-fingerprinting purposes. As a result, I'm much more comfortable with an explicit toggle persisted via a cookie or local storage. Using that query also has the unfortunate downside of assuming light mode as the default, which often results in that 'light flash' for dark mode users that I refer to in the comments!
… slight isual tweaks
/> | ||
<li> | ||
<span> | ||
<label class="toc-toggle" for="{{ subdir.path }}" onclick="event.stopPropagation()"> |
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.
Have you looked at the <details>
element for these kinds of disclosure widgets?
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 have played around with this, but the label/checkbox pattern worked better for my purposes. details elements don't allow you to customise transitions.
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.
What transition are you trying to customise? The rotating disclosure arrow?
It looks like you're already rendering the arrow with a ::before
pseudo-element and then animating its transition with a sibling selector on input:checked
. You should be able to do the same with <details>
by removing the default icon and using details[open]
to differentiate opened and closed elements.
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 not sure there's much of an advantage to using details here instead. For better or for worse, the label approach is what I'm more familiar with. Maybe we can address this in a follow-up PR later on, if we realise there's a significant advantage?
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.
Fair to split it as we still use the input:checked
trick in the site-wide header but have started to prefer the native <details>
for new designs. The thing worth exploring is whether there is an accessibility benefit to the latter or, more easily tested, if it improves the site behaviour in browser “reader mode” (as you’re no longer reliant on CSS alone to control the disclosure behaviour).
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.
Makes perfect sense, thanks for the explanation!
Reader mode seems to ignore the sidebar entirely (fair, since it isn't core content), but at screen widths where the on-this-page element displays, we do wind up with a big list of headers at the end of the reader mode document.
In a future update, I'll explore switching to details
elements for the sidebar and also look into javascript-free alternatives to the current on-this-page widget. IMO it's OK for such a ancillary element to dispense accessibility, but preferably I'd find a better way that's more compatible with reader mode as well. Ultimately it would be best to eliminate tabs from the landing page and simplify the site rendering process, but I think we're iterating in the right direction for now. Just don't ask me about duplicate header names on the same (long) page -- there doesn't seem to be a great way to handle those right now.
Another thing to consider is that any changes to the page markup may require a change to our Algolia DocSearch Crawler which currently uses the following selectors to extract the various "levels" of documentation for search results:
(Note the Pico SDK only uses levels 0-2 from above and uses |
Good reminder. I think this should all be OK though -- I have left the |
Aren't the tabs gone? Replaced with a full TOC instead? Perhaps there is an updated selector we can use to inform the crawler which top-level section they're in? |
Hmmm. Might be that I'm misunderstanding how search indexing works with the tabs. I'll take a look. |
…branding updates to the landing page and header
@mudge the current content hierarchy looks like: html
body
#docs-content
#docs-container
#main-window
#content
h1
.sect1
h2
.paragraph
<content>
.sect2
h3
.paragraph
<content>
.sect3
h4
.paragraph
<content>
.sect4
h5
.paragraph
<content> Obviously admonitions, code blocks, definitions, tables, tabs, and more break that hierarchy up a bit, but hopefully that gives you the gist! I can always provide more information about those elements if you'd like to index those as well and you need more info. |
… with, with centered content, to prevent sidebar from drifting too far to left on ultrawide displays
…ext; switch back to always-left sidebar location
Cleaned up a LOT of HTML and CSS.
Added a dark theme for the entire docs site.
Improved responsiveness and load times for the site in general.
Uses space more effectively on desktop and mobile.
Sample images:
Dark desktop:
Light desktop:
Light mobile:
Dark mobile:
Light mobile menu:
Dark mobile menu: