Skip to content

feat: add playful 404 and error pages #3248

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Oct 15, 2024
Merged

feat: add playful 404 and error pages #3248

merged 11 commits into from
Oct 15, 2024

Conversation

andre-code
Copy link
Contributor

@andre-code andre-code commented Jul 19, 2024 •
edited
Loading

Playful 404 and error pages

Cases to test:
Cases current Renku

Cases Renku 2.0

Screenshots
NOT FOUND GROUP

Screenshot 2024-07-26 at 14 21 37
Screenshot 2024-07-26 at 14 21 18

Screenshot 2024-07-26 at 14 20 44
Screenshot 2024-07-26 at 14 20 29

fix #3235

/deploy

@RenkuBot
Copy link
Contributor

You can access the deployment of this PR at https://renku-ci-ui-3248.dev.renku.ch

@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 19, 2024 13:08 — with GitHub Actions Inactive
@andre-code andre-code force-pushed the andrea/error-pages-ui branch 3 times, most recently from 914dc79 to 6171132 Compare July 26, 2024 07:40
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 26, 2024 07:40 — with GitHub Actions Inactive
@andre-code andre-code force-pushed the andrea/error-pages-ui branch from 6171132 to 1192840 Compare July 26, 2024 11:15
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 26, 2024 11:15 — with GitHub Actions Inactive
@andre-code andre-code force-pushed the andrea/error-pages-ui branch from 1192840 to f7d54a4 Compare July 26, 2024 12:23
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 29, 2024 11:43 — with GitHub Actions Inactive
lorenzo-cavazzi
lorenzo-cavazzi reviewed Jul 29, 2024
View reviewed changes
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

This looks nice!
A couple of comments before digging deep into the code:

  • Some content is flashing on the top left when opening a link like the following (see gif)
    https://renku-ci-ui-3248.dev.renku.ch/projects/fakename
    Peek 2024-07-29 14-55

  • The Error page is blank, except for the footer. I would suggest removing it so that we minimize the probably of crashing the error page itself.
    The content is also centered vertically, while it's not for the other error pages; I suggest keeping it consistent, adding only some margin on top
    image

  • The custom CSS classes should be limited to the bare minimum and avoided wherever unnecessary. E.G. We can use the bootstrap w-100 instead of width: 100%;

@lorenzo-cavazzi
Copy link
Member

P.S. Sorry I missed that this PR was still a draft 🤦
I hope the comments help anyway

Base automatically changed from lorenzo/bootstrap-design to main July 30, 2024 09:05
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 30, 2024 11:35 — with GitHub Actions Inactive
@andre-code andre-code force-pushed the andrea/error-pages-ui branch from f7d54a4 to 631645f Compare July 30, 2024 11:51
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 30, 2024 11:52 — with GitHub Actions Inactive
@andre-code
Copy link
Contributor Author

Thanks, @lorenzo-cavazzi, for your helpful suggestions.

Regarding the flashing information, I'm unsure how to address it. The info is the loading message indicating what is being searched for, but since, in the best case, this query is resolved quickly, it shows our 'not found' page in seconds. There isn't enough time to read the loading message.

So, I am not sure whether to delay the query response or leave it as it is. Do you have a suggestion for this case?
I've already included your other suggestions in the latest commit.

@andre-code andre-code force-pushed the andrea/error-pages-ui branch from 30ad2ab to b9e0417 Compare July 30, 2024 14:45
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 July 30, 2024 14:45 — with GitHub Actions Inactive
@andre-code andre-code marked this pull request as ready for review July 30, 2024 15:34
@andre-code andre-code requested a review from a team as a code owner July 30, 2024 15:34
@andre-code andre-code temporarily deployed to renku-ci-ui-3248 August 2, 2024 08:21 — with GitHub Actions Inactive
@lorenzo-cavazzi lorenzo-cavazzi self-assigned this Aug 13, 2024
lorenzo-cavazzi
lorenzo-cavazzi requested changes Aug 13, 2024
View reviewed changes
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Great to see these components being standardized! 🙌

I noticed a few sub-optimal solutions (see inline comments -- and please feel free to justify your changes if you feel I missed the reason).
Even tho they mostly do not impact the users, we should try to avoid bad patterns we used in the past (like prop drilling, inconsistent spaces, unnecessary abstraction layers) so that it's easier to maintain the code in the long term 🙂

client/src/not-found/OopsImage.tsx Outdated
Comment on lines 24 to 26
export default function OopsImage({ className, id }: OopsImageProps) {
return (
<svg viewBox="0 0 211.64 54.61" className={className} id={id}>
Copy link
Member

Choose a reason for hiding this comment

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

Same here. You can add IDs and add classes straight to the img instead of passing them down to the additional level of abstraction

client/src/features/projectsV2/notFound/ProjectNotFound.tsx Outdated
Comment on lines 72 to 75
<div className={cx("d-flex", "justify-content-center", "w-100")}>
<div className={cx(styles.errorNotFoundContainer, "m-auto")}>
<Row>
<Col className={cx("p-4", "mt-5")}>
Copy link
Member

Choose a reason for hiding this comment

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

There is an interesting combination of classes here 😄

  • The custom style interferes with automatic margins forcing the content to a specific width so that on larger screens there is an additional white space on the right.
    image
    I suggest dropping that div entirely, together with the fixed width, and let m-auto do its job.
    What's the reason for the fixed width? If you wish to avoid wrapping, you could use text-nowrap.
  • There is an additional "row-col" layer that feels unnecessary here. Perhaps that's a leftover after trying a multi-column layout?
  • There are multiple classes that are adding space to the top. The final result is a space between the top border and the first component that is a combination of mt-5 + p-4 + mt-3, ending up with a semi-random number of pixels. I suggest avoiding this pattern because it makes keeping spacing consistent throughout the application much harder.
    In this case, you could remove the classes from the children components and let the topmost component handle also the space on top.

Copy link
Member

Choose a reason for hiding this comment

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

Please double-check the other pages that implement a similar pattern. This is used in several places in this PR

@andre-code andre-code force-pushed the andrea/error-pages-ui branch from c22d815 to ef01169 Compare October 14, 2024 09:38
@andre-code andre-code force-pushed the andrea/error-pages-ui branch from ef01169 to 014abfe Compare October 14, 2024 09:44
@lorenzo-cavazzi
Copy link
Member

@andre-code looks good now! There is something off with "oops" images used for the error pages. In my case, they don't show up.

Screenshot_20241015_091835

It's probably just something with the svg attributes cause I see the image in my file explorer, but when I try to open it with Firefox it doesn't show up and I see the following message:

`This XML file does not appear to have any style information associated with it.

leafty
leafty requested changes Oct 15, 2024
View reviewed changes
client/src/error-boundary/ErrorBoundary.tsx Outdated
{children}
</Sentry.ErrorBoundary>
);
}

function ErrorPage() {
const ErrorPage = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Use the function coding style here.

client/src/error-boundary/ErrorBoundary.tsx Outdated
return (
<Sentry.ErrorBoundary onError={onError} fallback={ErrorPage}>
<Sentry.ErrorBoundary onError={onError} fallback={fallbackErrorPage}>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I get why you cannot just use ErrorPage here.

lorenzo-cavazzi
lorenzo-cavazzi approved these changes Oct 15, 2024
View reviewed changes
Copy link
Member

@lorenzo-cavazzi lorenzo-cavazzi left a comment

Choose a reason for hiding this comment

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

Lgtm 🚀

@andre-code andre-code merged commit 0eebdcb into main Oct 15, 2024
20 checks passed
@andre-code andre-code deleted the andrea/error-pages-ui branch October 15, 2024 12:25
@RenkuBot
Copy link
Contributor

Tearing down the temporary RenkuLab deplyoment for this PR.

leafty pushed a commit that referenced this pull request Apr 22, 2025
@andre-code @leafty
feat: add playful 404 and error pages (#3248)
4836dbc 
fix #3235
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leafty leafty leafty approved these changes

@lorenzo-cavazzi lorenzo-cavazzi lorenzo-cavazzi approved these changes

Assignees

@lorenzo-cavazzi lorenzo-cavazzi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Playful 404 and error pages
4 participants
@andre-code @RenkuBot @lorenzo-cavazzi @leafty