-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add the Blue Bar #41
Add the Blue Bar #41
Conversation
9dca5b2
to
dd110a6
Compare
Noting that there currently isn't a very good way to show the "All Posts" title, which should appear in the Blue Bar on the Posts page if the site is configured to have a static homepage. Ideally we'd use the The alternative for now, I think, is to copy the |
On second thought, the "All Posts" heading block could probably live outside the header.html block template part. So the home.html file would be identical to index.html, but with the addition of a heading block above the |
source/wp-content/themes/wporg-news-2021/block-template-parts/header-navigation.html
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-news-2021/block-template-parts/header-navigation.html
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-news-2021/sass/post/_header.scss
Outdated
Show resolved
Hide resolved
i'll check when i get into the code, but I generally prefer that tradeoff too, seems less fragile and more straightforward. might be more reliable for screen readers too?
Another idea would be to have both |
I guess the question is if the global header will always be fixed, or if that's specific to the News site...
🤔 That might work. I'll try 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.
Left some comments, but no blockers 👍🏻
source/wp-content/themes/wporg-news-2021/sass/post/_header.scss
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-news-2021/sass/post/_header.scss
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
&-toggle { |
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 partial class names feels kind of awkward to me; if I don't remember what the containing SCSS block is named, then I have to scroll back up to the top to find out.
PhpStorm does show it in its breadcrumb toolbar, but it still feels less readable to me, especially in long blocks like this.
I don't feel strongly though.
source/wp-content/themes/wporg-news-2021/block-template-parts/header-navigation.html
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-news-2021/sass/post/_header.scss
Outdated
Show resolved
Hide resolved
The former seems like a safe assumption. @beafialho, is that what you were thinking? |
Co-authored-by: Ian Dunn <[email protected]>
Yes @iandunn I was thinking the global header would be fixed for all the site's pages. |
I was just reading this, and thinking that after all this, it might actually be simpler and more robust to use CSS grid for the layout of this bar and manage the positions of things responsively. It might not be worth the refactor time at this point, though. @iandunn I'm going to go ahead and merge this, and you can revisit with CSS grid if layout bugs crop up later. |
Introduces the "Blue Bar" or, more accurately, header navigation. It includes a simple taxonomy-based breadcrumb on the left and a dropdown menu of categories on the right. The color scheme changes ("The Not Blue Bar") for certain categories. The layout responsively handles mobile, tablet, and desktop views.
The categories dropdown functionality is entirely CSS-based. I'm not sure if this is the right approach, as it made the stylesheet a bit more complex, but it's nice that no JS file is required.
This also makes the entire site header (including the global header) fixed to the top when not in a narrow mobile viewport, as called for in the design.
This does not yet handle the "All Posts" large heading for the Posts Page view, due to the issue described below.
Fixes #32