-
Notifications
You must be signed in to change notification settings - Fork 15
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 breadcrumbs component #459
Conversation
775509e
to
da3ae15
Compare
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.
<a href="/about" className={clsx('hover:text-bluedot-normal', isCurrentPath('/about') && 'font-bold')}>About us</a> | ||
<a href="/careers" className={clsx('hover:text-bluedot-normal', isCurrentPath('/careers') && 'font-bold')}>Join us</a> | ||
<a href={ROUTES.about.url} className={clsx('hover:text-bluedot-normal', isCurrentPath(ROUTES.about.url) && 'font-bold')}>About us</a> | ||
<a href={ROUTES.careers.url} className={clsx('hover:text-bluedot-normal', isCurrentPath(ROUTES.careers.url) && 'font-bold')}>Join us</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.
I've attempted to replace all hardcoded strings used for internal navigation, so if you find any I have missed please let me know
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.
a few small notes. will get Vio's eyes on this tomorrow!
apps/website-25/src/lib/routes.ts
Outdated
|
||
export const ROUTES = { | ||
home: { | ||
title: 'BlueDot Impact', |
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 designs have this as "Bluedot.org" but I'm also thinking "Home" might be best? will sync with PM tomorrow to confirm
title: 'BlueDot Impact', | |
title: 'Home', |
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.
Cool, just let me know 👍. I would like to try and keep the tab title the same if possible, which is why I called it "BlueDot Impact" instead of "Bluedot.org" (sorry I forgot to flag that). So if changing it to "Home" I think it would make sense to change the title from:
<title>{CURRENT_ROUTE.title} | Industry-leading free AI courses and career support</title>
to:
<title>{CURRENT_ROUTE.title} | BlueDot Impact</title>
import { BluedotRoute, ROUTES } from '../../lib/routes'; | ||
|
||
export const CURRENT_ROUTE: BluedotRoute = { | ||
title: 'AI Safety Teaching Fellow', | ||
url: `${ROUTES.careers.url}/ai-safety-teaching-fellow`, | ||
}; |
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 actually quite like colocating the route def in the /pages/
route file itself. what do you think about doing the same for all pages?
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.
sepearate idea: can we write a test that:
- uses node:fs to get all the pages recusively in the pages directory
- uses
await import()
to dynamically import them, and get the CURRENT_ROUTE - check that the url matches what it should based on the Next.js routing logic
this should enforce that the url is actually correct.
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 actually quite like colocating the route def in the /pages/ route file itself. what do you think about doing the same for all pages?
I agree this would be better if possible. The reason I didn't do that is that it creates the potential for circular dependencies: A parent page is likely to contain a link to a child page, so will need to import it, and then the child will need to import the parent to reference the route in the breadcrumbs. I did it in this case because these are known to be "leaf" pages that don't have further children.
In the current site we get a dependency cycle between /careers
and these two specific job listings if we define the route inside the careers.tsx
file.
We could add some registerRoute
step that puts the route into a variable that is accessible globally (or use declare global
in the file directly), but I thought that for the current scope of the website maintaining this separate file is simpler and isn't too much of a hassle
* Relative url of the route (e.g. /about, not https://bluedot.org/about) | ||
*/ | ||
url: string; | ||
}; |
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 about including the definition of parent page(s) within the BluedotRoute type, with the default pointing to home
?
parentPages: [BluedotRoute]
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.
im concerned that requiring consumers to manually set the path introduces a fragile pattern, so i'm trying to minimize that where we can...
i poked around a little bit, and i'm also not convinced that dynamically generating these paths is a robust enough solution to this. e.g. this one breaks when we introduce page titles that differ from their route name (e.g. /about vs About Us) 😅 i think your proposal works great and serves our purposes 💯
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 about including the definition of parent page(s) within the BluedotRoute type
Great idea 😌, this does simplify things a lot. I tried also setting the deafult the default as home
by adding a function like this:
export function defineRoute({ title, url, childOf = home }: { title: string; url: string; childOf?: BluedotRoute }): BluedotRoute {
return {
title,
url,
parentPages: childOf ? [...(childOf.parentPages ?? []), childOf] : [],
};
}
Again I ended up thinking that given the number of pages currently on the site it's not worth the extra complexity to have this function, so I have kept it that you have to referece home
manually`:
const about: BluedotRoute = {
title: 'About us',
url: '/about',
parentPages: [home],
};
libraries/ui/src/CookieBanner.tsx
Outdated
@@ -5,6 +5,7 @@ import posthog from 'posthog-js'; | |||
import { CTALinkOrButton } from './CTALinkOrButton'; | |||
|
|||
type CookieBannerProps = { | |||
cookiePolicyUrl: string, |
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 this is probably not worth parameterising. We only have one privacy policy across everything, so hard coding it within the component seems fine (possibly to bluedot.org/privacy-policy if we expect to use it across different apps and we're worried the relative path won't work)
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.
LGTM, would be happy to merge as-is and for you to make the changes in a follow-up
Thanks for the reviews 😄. I'll merge as is as Adam suggested and followup later based on the open discussions |
Summary
Issue
Fixes #397
Description
Developer checklist
Screenshot
Testing