Skip to content

refactor: Fix header styling issues #1292

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.

Sign up for GitHub

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

Merged
rschristian merged 7 commits into from
Jun 11, 2025
Merged

refactor: Fix header styling issues #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
309aea6
refactor: Simplify i18n color change on drop down
rschristian Jun 10, 2025
16a9377
refactor: Sidebar toggle should be below expanded header
rschristian Jun 10, 2025
6ae52ba
fix: Stop nav links in header from wrapping
rschristian Jun 10, 2025
5fd605e
fix: Ensure header dropdown arrow is vertically centered
rschristian Jun 10, 2025
f797bf3
fix: Social items in header
rschristian Jun 10, 2025
410c2eb
fix: Specificity
rschristian Jun 11, 2025
297aaf0
fix: Dropdown arrow alignment in expanded header
rschristian Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/assets/icons.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
46 changes: 29 additions & 17 deletions src/components/header/style.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
background: #38235c;
color: white;

height: var(--banner-height);

padding: 0.5rem;

a {
Expand Down Expand Up @@ -29,7 +31,7 @@
.open & {
justify-content: space-between;
-webkit-overflow-scrolling: touch;
height: var(--vh);
height: calc(var(--vh) - var(--banner-height));
Comment on lines -32 to +34

@rschristian rschristian Jun 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Predates my involvement, but IIUC, this was a problem introduced when the banner was added. It seems like the idea was that the social icons would be at the bottom of the screen when the togglable header was expanded, however, it never took into account the banner height. As such, the icons were always clipped off the screen.

The handful of white pixels at the very bottom of the screen are our icons, but you obviously can't interact with them as-is.

Screenshot of the site on a mobile device with the header expanded. Shows just the very top of icons (a handful of pixels at most)

}
}

Expand Down Expand Up @@ -94,11 +96,7 @@
}

nav {
position: relative;
display: inline-block;
font-size: 100%;
user-select: none;
-webkit-user-drag: none;
Comment on lines -97 to -101

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAICT, completely unnecessary.

display: flex;

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixes the issue of header items wrapping on some screensizes:

Screenshot of the site at a width of 900px. The header nav items wrap so that the 'REPL' link is pushed out of the header and on top of the page content

Still not perfect, there's now some overlapping in the header, but wrapping is very bad.


@media (max-width: /* --header-mobile-breakpoint */ 50rem) {
display: block;
Expand Down Expand Up @@ -185,6 +183,8 @@

& > a,
& > button {
display: flex;
align-items: center;
Comment on lines +186 to +187

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ensures the little dropdown arrow next to 'About' (or any future dropdown pages) is vertically centered:

Screenshot of the site header. There's a triangular dropdown arrow next to the 'About' link, but it's a bit lower than center compared to the label

transition: all 0.3s;

&::after {
Expand All @@ -203,7 +203,7 @@

& > a,
& > button {
display: block;
display: flex;
}
}

Expand Down Expand Up @@ -255,12 +255,6 @@
& > button {
background: #fff;
color: #555;
z-index: 101;

svg {
filter: invert(40%) sepia(0%) saturate(3773%) hue-rotate(315deg)
brightness(73%) contrast(81%);
}
Comment on lines -258 to -263

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The z-index is entirely unused, goes back to the repo creation. This style would only apply to the 'About' or i18n dropdown buttons which wouldn't need a z-index as they're kept inline, exactly where they were.

The SVG filter is a far more complex method of updating the icon color; way easier to just switch the icon to use currentColor and piggyback off the color setting just a few lines up (color: #555).

}

nav {
Expand Down Expand Up @@ -313,6 +307,8 @@
justify-content: space-between;
align-items: flex-start;

touch-action: none;

Comment on lines +310 to +311

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

When the header is opened, we should not let the user scroll the page in the background.

@media (min-width: /* --content-width */ 50rem) {
justify-content: center;
justify-content: flex-start;
Expand All @@ -327,21 +323,38 @@
.translation {
position: absolute;
height: 2rem;
top: calc(var(--vh) - 3rem); /* TODO: Why does bottom not work? */
display: flex;
display: none;
bottom: calc(env(safe-area-inset-bottom) + 1rem);
Comment on lines -330 to +327

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hides the icons by default and uses the more straightforward bottom now that our parent height has been corrected (see comment above).

What it looks like now:

Screenshot of header expanded on mobile. The social icons now sit within the viewport at the bottom of the screen

width: 100%;
justify-content: space-between;
align-items: center;

@media (max-width: /* --header-mobile-breakpoint */ 50rem) {
padding-left: 26px;
padding-right: 26px;
}

.open & {
display: flex;
}

@media (min-width: /* --content-width */ 50rem) {
top: initial;
display: flex;
bottom: initial;
height: 100%;
left: initial;
position: relative;
width: initial;
}
}

.translation {
@media (max-width: /* --header-mobile-breakpoint */ 50rem) {
/* Otherwise overlaps the social items in the dropdown header, blocking use */
display: none !important;
}
}

.socialItem {
padding: 0 0.5rem;
}
Expand Down Expand Up @@ -399,7 +412,6 @@
padding: 18px 23px 12px;
line-height: var(--header-height);
text-align: center;
z-index: 100;

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Superfluous z-index, it sits on the header (z-index of 500 already) and nothing overlaps it either.

opacity: 0.7;
cursor: pointer;
transition: opacity 250ms ease;
Expand Down
2 changes: 1 addition & 1 deletion src/components/sidebar/style.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
color: #fff;
fill: rgba(255, 255, 255, 0.6);
cursor: pointer;
z-index: 560;
z-index: 460;

@rschristian rschristian Jun 10, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think this was a typo.

The slide-out sidebar has a z-index of 450, and the slide-down header has a z-index of 500. As such, when the header is expanded, there's really no reason for the sidebar toggle button to be displayed -- you're toggling something that can't be seen.

z-index of 460 allows it to still sit on top of the sidebar when expanded but hidden behind the header if it is expanded.

transition: transform 0.3s;

&:after {
Expand Down
3 changes: 2 additions & 1 deletion src/style/variables.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
:root {
--vh: 100vh;
--banner-height: 2.5rem;
--header-height: 3.5rem;
--header-mobile-breakpoint: 50rem;
--header-and-banner-height: calc(var(--header-height) + 2.5rem);
--header-and-banner-height: calc(var(--header-height) + var(--banner-height));
--content-width: 50rem;
--sidebar-break: 900px;

Expand Down