Skip to content
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

fix(404): add button for searches #869

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ const createInfobarStyles = tv({
},
})

const compoundStyles = createInfobarStyles()
export const compoundStyles = createInfobarStyles()

const Infobar = ({
title,
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export { default } from "./Infobar"
export { default, compoundStyles } from "./Infobar"
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { type NotFoundPageSchemaType } from "~/engine"
import { renderComponent } from "../../render"
import { getTailwindVariantLayout } from "~/utils"
import { compoundStyles } from "../../components/complex/Infobar/Infobar"
import { LinkButton } from "../../components/internal/LinkButton"
import { Skeleton } from "../Skeleton"
import { NotFoundSearchButton } from "./SearchButton"

const NotFoundLayout = ({
site,
Expand All @@ -9,7 +12,13 @@ const NotFoundLayout = ({
LinkComponent,
ScriptComponent,
}: NotFoundPageSchemaType) => {
const simplifiedLayout = getTailwindVariantLayout(layout)

return (
// NOTE: This is taken from Infobar in components.
// However, we duplicated it here so that we can set the
// search button as a client component and avoid streaming over a
// huge payload to our end user
<Skeleton
site={site}
page={page}
Expand All @@ -22,18 +31,57 @@ const NotFoundLayout = ({
// but cannot be used here as tailwind does not support dynamic class names
className={`[&_.component-content]:mx-auto [&_.component-content]:max-w-screen-xl [&_.component-content]:px-6 [&_.component-content]:md:px-10`}
>
{renderComponent({
component: {
type: "infobar",
title: "404: Page not found",
description: "Sorry, the page you were looking for cannot be found",
buttonLabel: "Go to homepage",
buttonUrl: "/",
},
layout,
site,
LinkComponent,
})}
<section>
<div
className={compoundStyles.outerContainer({
layout: simplifiedLayout,
})}
>
<div
className={compoundStyles.innerContainer({
layout: simplifiedLayout,
})}
>
<div
className={compoundStyles.headingContainer({
layout: simplifiedLayout,
})}
>
<h2
className={compoundStyles.title({ layout: simplifiedLayout })}
>
Page not found
</h2>

<p
className={compoundStyles.description({
layout: simplifiedLayout,
})}
>
This page might have been moved or deleted. Try searching for
this page instead.
</p>
</div>

<div
className={compoundStyles.buttonContainer({
layout: simplifiedLayout,
})}
>
<NotFoundSearchButton LinkComponent={LinkComponent} />
<LinkButton
href="/"
size="lg"
variant="outline"
LinkComponent={LinkComponent}
isWithFocusVisibleHighlight
>
Go to homepage
</LinkButton>
</div>
</div>
</div>
</section>
</div>
</Skeleton>
)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
"use client"

import { useLayoutEffect, useState } from "react"

import { NotFoundPageSchemaType } from "~/engine"
import { getWordsFromPermalink } from "~/utils"
import { LinkButton } from "../../components/internal/LinkButton"

type NotFoundSearchButtonProps = Pick<NotFoundPageSchemaType, "LinkComponent">
export const NotFoundSearchButton = ({
LinkComponent,
}: NotFoundSearchButtonProps) => {
const [permalink, setPermalink] = useState("")

useLayoutEffect(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need useLayoutEffect here? iirc, the use of this hook is discouraged in favour of useEffect

// The check for typeof window and navigator ensures this only runs in browser environments, not during server-side rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

this will never be server-side rendered due to "use client" declarative at top

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not server-side rendered but nextjs will transpile then serve the js at runtime. this led to compile errors as window is undefined when transpiling

if (
typeof window !== "undefined" &&
typeof window.location !== "undefined"
) {
setPermalink(window.location.pathname)
}
}, [])

const missingPath = getWordsFromPermalink(permalink)

return (
<LinkButton
href={`/search?q=${missingPath}`}
size="lg"
LinkComponent={LinkComponent}
isWithFocusVisibleHighlight
>
Search for this page
</LinkButton>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { describe, expect, it } from "vitest"

import { getWordsFromPermalink } from "~/utils"

describe("getWordsFromPermalink", () => {
it("should trim out all symbols and return the permalink as a space separated sentence for single level permalinks", () => {
// Arrange
const singleLevelPermalink = "/this-._single=level|"
const expected = "this+single+level"

// Act
const actual = getWordsFromPermalink(singleLevelPermalink)

// Assert
expect(actual).toBe(expected)
})

it("should trim out all symbols and return the last section as a space separated sentence for nested permalinks", () => {
// Arrange
const nestedPermalink = "/nested/deeply/this-._nest'fff=level|"
const expected = "this+nest+fff+level"

// Act
const actual = getWordsFromPermalink(nestedPermalink)

// Assert
expect(actual).toBe(expected)
})

it("should preserve `=` in the original permalink", () => {
// Arrange
const singleLevelPermalink = "/this-._single=level|"
const expected = "this+single+level"

// Act
const actual = getWordsFromPermalink(singleLevelPermalink)

// Assert
expect(actual).toBe(expected)
})
Comment on lines +30 to +40
Copy link
Contributor

Choose a reason for hiding this comment

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

unsure if i understand the test description correctly but = isn't preserve but converted?

Copy link
Contributor Author

@seaerchin seaerchin Nov 13, 2024

Choose a reason for hiding this comment

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

this is my bad, i was flipflopping on whether we should keep this due to query params but i decided teh = wasn't going to impact the search results much - i'll just remove this test and add a = character ot the other permalinks


it("should handle uri-encoded strings correctly", () => {
// Arrange
const singleLevelPermalink = "/this-._single=level|"
const encodedPermalink = encodeURIComponent(singleLevelPermalink)
const expected = "this+single+level"

// Act
const actual = getWordsFromPermalink(encodedPermalink)

// Assert
expect(actual).toBe(expected)
})

it("should work with a trailing /", () => {
// Arrange
const singleLevelPermalink = "/this-._single=level|/"
const expected = "this+single+level"

// Act
const actual = getWordsFromPermalink(singleLevelPermalink)

// Assert
expect(actual).toBe(expected)
})
})
15 changes: 15 additions & 0 deletions packages/components/src/utils/getWordsFromPermalink.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
export const getWordsFromPermalink = (permalink: string): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

checking understanding:

so https://example.com/some-page_name+ will yield some+page+name right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap excatly

const trimmedPermalink = permalink.endsWith("/")
? permalink.slice(0, -1)
: permalink
const lastUrlSegment = trimmedPermalink.split("/").at(-1) ?? ""
// NOTE: Replace all non-alphanumeric characters with spaces
// then remove all spaces and join by `+`.
// This is because we might have run-on spaces from sequences of symbols
// like: `+=`, which would lead to 2 spaces
return decodeURIComponent(lastUrlSegment)
.replaceAll(/[\W_]/gi, " ")
.split(" ")
.filter((v) => !!v)
.join("+")
}
1 change: 1 addition & 0 deletions packages/components/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,4 @@ export { isExternalUrl } from "./isExternalUrl"
export { orderedListSchemaBuilder } from "./orderedListSchemaBuilder"
export * from "./tailwind"
export { unorderedListSchemaBuilder } from "./unorderedListSchemaBuilder"
export { getWordsFromPermalink } from "./getWordsFromPermalink"
Loading