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

Add not found page #285

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add not found page #285

wants to merge 13 commits into from

Conversation

magdalenaxm
Copy link
Contributor

@magdalenaxm magdalenaxm commented Jul 5, 2024


PR Checklist

  • Link to the respective task if one exists: COM-806
  • Provide screenshots/screencasts if the change contains visual changes
Screenshots/screencasts

Screenshot 2024-08-06 at 07 04 00

@magdalenaxm magdalenaxm self-assigned this Jul 5, 2024
@auto-assign auto-assign bot requested review from dkarnutsch, fraxachun and nsams July 5, 2024 07:37
@johnnyomair
Copy link
Collaborator

Please change the base to main.

@thomasdax98 thomasdax98 mentioned this pull request Jul 24, 2024
1 task
@thomasdax98 thomasdax98 changed the base branch from next to main August 5, 2024 19:33
@magdalenaxm magdalenaxm changed the title Add not found page !283 Add not found page Aug 6, 2024
import { IntlProvider } from "@src/app/[domain]/[language]/IntlProvider";
import { GQLLayoutQuery, GQLLayoutQueryVariables } from "@src/app/[domain]/[language]/layout.generated";
Copy link
Member

Choose a reason for hiding this comment

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

please use relative import

Copy link
Member

Choose a reason for hiding this comment

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

nsams
nsams previously approved these changes Aug 6, 2024
@thomasdax98 thomasdax98 requested review from nsams and johnnyomair and removed request for thomasdax98 August 6, 2024 07:43
@thomasdax98 thomasdax98 force-pushed the add-not-found branch 2 times, most recently from dba5772 to efcea3b Compare August 6, 2024 12:43
/>
</Typography>
<Typography variant="p200">
<Link href={`/${language}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this link always work? Because of this code in layout.tsx:

    if (!siteConfig.scope.languages.includes(language)) {
        notFound();
    }

Copy link
Member

Choose a reason for hiding this comment

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

The layout uses the not found page of the layer above, so it falls back to the Next.js default:

Bildschirmfoto 2024-08-07 um 11 20 55

We could also add an extra not found page for that. The problem is, we can't use FormattedMessage there because we have no IntlProvider since we don't know which language we are in.

Right now, we only have a not found page within the language scope. We could also add one for fallback which contains hardcoded English

@dkarnutsch dkarnutsch removed their request for review August 13, 2024 06:43
@johnnyomair johnnyomair mentioned this pull request Sep 16, 2024
2 tasks
@fraxachun
Copy link
Contributor

Please consider #363 as additional context.

@johnnyomair
Copy link
Collaborator

@thomasdax98 what's the status here?

@thomasdax98
Copy link
Member

@johnnyomair I simplified it once more. Now it's ready from my perspective

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's odd that we need an additional component to fetch the header data for the non-internationalized not found page. Wouldn't it be better if the not found page fetched the header and passes it to the component, as we do in the layout?

Copy link
Member

Choose a reason for hiding this comment

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

If I do that, I need to make the query in the not found page and in the layout. I wanted to avoid the code duplication, so I moved it to this shared component. I can't put it directly in the Header since it's a client component.

Would you prefer the code duplication?

Copy link
Collaborator

@johnnyomair johnnyomair Sep 30, 2024

Choose a reason for hiding this comment

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

Would you prefer the code duplication?

I'm not sure. I'd prefer to have the solution that's the most easy to understand. It took me a while to understand your change.

Would it be possible to reuse the layout from /[domain]/[language]/layout.tsx in /[domain]/layout.tsx with a default language?

Copy link
Member

Choose a reason for hiding this comment

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

Moving (almost everything) from layout to Header might not be ideal: in real world applications the layout contains probably more than just a header, for example a footer.

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.

5 participants