Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Actual mobile toc; dark mode; speed up site loading #3950
Changes from all commits
18f7b8f
0da2ebf
2bbe65f
90e30af
2881b23
b161535
32dcbc5
d3b7019
f71186f
da42c69
49d898d
d2ccf7c
cb1bd36
0103af0
72da5ec
0be1310
50ba5b6
54f5e4d
a28a588
0d43af2
673a53c
49bf5b8
6716e93
c82e538
ec51bb4
182494f
09738a5
f55d1e0
1bc73ff
f9e443f
5ec4770
11c7bcb
2bb249a
f945ba9
fd41a86
2ed5d08
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This file was deleted.
This file was deleted.
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
andfooter.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).
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.