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

feat: introduced not found / error layout #6247

Merged
merged 11 commits into from
Jan 17, 2024

Conversation

canerakdas
Copy link
Member

@canerakdas canerakdas commented Jan 15, 2024

Description

This PR introduces not found / error layout

Screenshot

image image

Validation

  • The not found page should work on local deployments with npm run serve:redesign
  • The design must match with Figma

@canerakdas canerakdas added website redesign Issue/PR part of the Node.js Website Redesign feature-request Requesting a new Technological Feature to be added to the Website labels Jan 15, 2024
Copy link

vercel bot commented Jan 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 17, 2024 3:38pm

@AugustinMauroy

This comment was marked as off-topic.

@ovflowd

This comment was marked as off-topic.

@AugustinMauroy

This comment was marked as off-topic.

@ovflowd

This comment was marked as off-topic.

@ovflowd
Copy link
Member

ovflowd commented Jan 15, 2024

I'm not sure about the implementation of the content, whether it should be read from a markdown file or translated from a localization file, so I'm opening it as a draft PR

Ideally through a Markdown file. Realistically, it's complex (sorta) but doable

For static exports, once the Website Redesign is done, is imperative that the structure and content of 404.md are able to render what the Not Found Layout does, hence it'd might make sense to actually replace 404.md into 404.mdx and move the content over there. So you don't need en.json i18n strings.

But that has a few caveats:

  • You'd need to import our MDX renderer and load the 404.mdx file from the not-found.tsx
    • Something like:
    const { MDXContent } =
      await dynamicRouter.getMDXContent(source, filename);
    
    <WithLayout layout="notFound">
      <MDXRenderer Component={MDXContent} />
    </WithLayout>
    • This allows you to add the layout to our WithLayout
    • You can make the Not Fund Component a Server Component (async) and do all this await inside the Component body.
    • FYI you might want for the meantime have a new file pages/en/new-design/404.mdx and have a conditional check of const notFoundSource = ENABLE_WEBSITE_REDESIGN ? 'pages/en/new-design/404.mdx' : 'pages/en/404.md';
  • For the global error page, you can't really use a Markdown file, so you'd need to make i18n strings (this is the global error: https://github.com/nodejs/nodejs.org/blob/main/app/global-error.tsx)

@ovflowd
Copy link
Member

ovflowd commented Jan 15, 2024

cc @canerakdas ☝️ my comment above has a relevant solution.

app/[locale]/not-found.tsx Outdated Show resolved Hide resolved
components/withLayout.tsx Outdated Show resolved Hide resolved
@ovflowd ovflowd marked this pull request as ready for review January 16, 2024 01:19
@ovflowd ovflowd requested a review from a team as a code owner January 16, 2024 01:19
Copy link

github-actions bot commented Jan 16, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟠 81 🟢 100 🟢 92 🟢 92 🔗
/en/about 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/about/previous-releases 🟢 98 🟢 96 🟢 92 🟢 92 🔗
/en/download 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/blog 🟢 100 🟢 97 🟢 92 🟢 92 🔗

Copy link

github-actions bot commented Jan 16, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
87.19% (395/453) 77.33% (116/150) 79.54% (70/88)

Unit Test Report

Tests Skipped Failures Errors Time
72 0 💤 0 ❌ 0 🔥 4.401s ⏱️

layouts/New/Centered.tsx Outdated Show resolved Hide resolved
app/[locale]/error.tsx Outdated Show resolved Hide resolved
app/[locale]/error.tsx Outdated Show resolved Hide resolved
app/global-error.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work here! ✨

@ovflowd ovflowd added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
@ovflowd ovflowd merged commit bd543e0 into nodejs:main Jan 17, 2024
16 checks passed
@canerakdas canerakdas mentioned this pull request Jan 22, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Requesting a new Technological Feature to be added to the Website website redesign Issue/PR part of the Node.js Website Redesign
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants