Skip to content

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Jul 31, 2025

Forewarning, sorry, this is fairly big.

Refactors most of the header to make everything a bit more fit for purpose but there are no real functional changes. It corrects a few (IMO) minor grievances that have popped up whilst making other edits:

  • Our usage / needs have drifted over the years compared to when most of components were built. Plenty of this is my doing, but for example:
    • Whilst we had a <NavMenu> for the "About" routes, we later added a language drop down. This overlapped somewhat with the drop-down menu, but not completely, so there's this awkward ternary in the component.
    • We needed to list /branding in config.nav so that it was detected as a valid route (404 detection and whatnot), but that list makes up the header items, so we had to add a magic prop
    • The "About" menu is sorta a link, sorta not, so we add an additional conditional to give it special handling too. As such, sometimes <NavLink> returns an actual link, sometimes it's just a button that triggers no navigation whatsoever. A bit weird.
    • We have a pathMatchesRoute function to apply the "selected" header style, but then have added additional conditionals outside of that because the function doesn't actually cover the cases we need.
    • TLDR: We've quickly appended stuff over the years. Happens in every project, but this should hopefully be a bit better for us & easier to adjust in the future when needed.
  • Starts to break up config.json's responsibilities
    • Our config.json is a bit of a weird god-file, in that it controls templating (config.nav -> header links), route definitions (used in /lib/route-utils.js for 404 detection & prerendering), and internationalization, all at once.
    • As such, if you wanted to add a class name to the "Home" link in the header, you're doing it via JSON properties, not the JSX. Until you need to use variables/components, that is, then you're left matching routes.
  • Language picker is inaccessible
    • My bad, the picker items are spans missing tabindexes, not that tabindexes there would be preferable over the alternatives anyhow. Can't select language via tabbing through the doc.
    • Swapped the spans for buttons, easy fix at least. Dunno what I was thinking there.

The responsibilities of config.json is still a problem that still needs work, but this has at least gotten the templating out for the most part -- no where else are we really passing props via JSON. Ideally, I'll get routes split out from the internationalization in a sane way and then write a plugin to split the i18n file per language, should be about ~5kb of savings there. You'll only need 1 locale at a time of course.

This ships a tiny bit less JS, but nothing stellar. ~1kb less.

button,
span {
button {
Copy link
Member Author

@rschristian rschristian Jul 31, 2025

Choose a reason for hiding this comment

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

The span styles only existed for the (now altered) LanguagePicker

Comment on lines -462 to +460
&[open] {
&[data-open='true'] {
Copy link
Member Author

@rschristian rschristian Jul 31, 2025

Choose a reason for hiding this comment

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

For TS, and we use data-open for every other menu too. Previously the hamburger menu used just open on the container div.

return out;
return out || null;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to avoid empty class attributes sitting around in prerendered HTML, more than anything.

> :information*desk_person: You [don't \_have* to use ES2015 to use Preact](https://github.com/developit/preact-without-babel)... but you should. This guide assumes you have some sort of ES2015 build set up using babel and/or webpack/browserify/gulp/grunt/etc. If you don't, start with [preact-cli](https://github.com/preactjs/preact-cli) or a [CodePen Template](http://codepen.io/developit/pen/pgaROe?editors=0010).
> :information_desk_person: You [don't _have_ to use ES2015 to use Preact](https://github.com/developit/preact-without-babel)... but you should. This guide assumes you have some sort of ES2015 build set up using babel and/or webpack/browserify/gulp/grunt/etc. If you don't, start with [preact-cli](https://github.com/preactjs/preact-cli) or a [CodePen Template](http://codepen.io/developit/pen/pgaROe?editors=0010).
Copy link
Member Author

@rschristian rschristian Jul 31, 2025

Choose a reason for hiding this comment

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

Sorry, unrelated but correcting a Prettier bug that screwed up the content.

Didn't want to bother w/ a separate PR for something so tiny.

Comment on lines -38 to +36
<Nav class={style.nav} routes={config.nav} current={url} />
<MainNav />
<Search />
<div class={style.social}>
<ReleaseLink class={cx(style.socialItem, style.release)} />
<SocialIcon
label="Browse the code on GitHub"
href="https://github.com/preactjs/preact"
viewbox="0 0 24 24"
id="github"
/>
<SocialIcon
label="Follow us on Twitter"
href="https://twitter.com/preactjs"
viewbox="0 0 34 27.646"
id="twitter"
/>
<SocialIcon
label="Follow us on Bluesky"
href="https://bsky.app/profile/preactjs.com"
viewbox="0 0 568 501"
id="bluesky"
/>
<SocialIcon
label="Chat with us on Slack"
href="http://chat.preactjs.com/"
viewbox="0 0 512 512"
id="slack"
/>
</div>
<div class={style.translation}>
<NavMenu language />
</div>
<SocialLinks />
<LanguagePicker />
Copy link
Member Author

@rschristian rschristian Jul 31, 2025

Choose a reason for hiding this comment

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

Broke this up as it was getting a bit big, though there's no complexity so equally I could merge everything back together too. Felt it looked marginally cleaner this way?

Comment on lines +84 to +116

/**
* Get the translated name of a path based upon the current language.
* @param {string} path
*/
export function useNavTranslation(path) {
const [lang] = useLanguage();

for (const route in config.nav) {
if (config.nav[route].path === path) {
return getRouteName(config.nav[route], lang);
} else if (config.nav[route].routes) {
for (const subRoute of config.nav[route].routes) {
if (subRoute.path === path) {
return getRouteName(subRoute, lang);
}
}
}
}

return null;
}

/**
* @param {{ name: Record<string, string> | string }} route
* @param {string} lang
* @return {string}
*/
export function getRouteName(route, lang) {
return typeof route.name === 'object'
? route.name[lang] || route.name.en
: route.name;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

More of a hold over mechanism, I hope to switch the nav translations into a keyed object & break them away from the route paths in the future.

@rschristian rschristian marked this pull request as ready for review August 4, 2025 04:48
@rschristian rschristian merged commit 353cc2a into master Aug 24, 2025
5 checks passed
@rschristian rschristian deleted the refactor/header branch August 24, 2025 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants