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 404 page #54

Merged
merged 15 commits into from
Nov 29, 2022
Merged

Add 404 page #54

merged 15 commits into from
Nov 29, 2022

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Nov 17, 2022

Fixes WordPress/wporg-showcase-2022#15

Screenshots

Desktop Tablet Mobile
Screen Shot 2022-11-23 at 5 42 10 PM Screen Shot 2022-11-23 at 5 42 23 PM Screen Shot 2022-11-23 at 5 42 31 PM

How to test the changes in this Pull Request:

  1. Set up a local env.
  2. Go to a random page, for example http://localhost:8898/random.
  3. Check the 404 page.

@renintw renintw added the [Component] Theme Templates, patterns, CSS label Nov 17, 2022
@renintw renintw self-assigned this Nov 17, 2022
@ryelle
Copy link
Contributor

ryelle commented Nov 17, 2022

Where did the design for this page come from? I can't find a 404 page design for showcase, and I'm wondering about the Oops color (it looks darker than I would expect). I also think this should just be using the global header & footer (rather than the template parts) to better match the news design.

@StevenDufresne
Copy link
Contributor

Where did the design for this page come from? I can't find a 404 page design for showcase, and I'm wondering about the Oops color (it looks darker than I would expect). I also think this should just be using the global header & footer (rather than the template parts) to better match the news design.

There isn't a design, but an agreement to use the same one as news. I think if we ship with the dark version would equally be ok, although a white version aligns better with the theme. I agree the oops text can be lightened.

@ryelle
Copy link
Contributor

ryelle commented Nov 18, 2022

There isn't a design, but an agreement to use the same one as news. I think if we ship with the dark version would equally be ok, although a white version aligns better with the theme. I agree the oops text can be lightened.

@jasmussen Can you chime in with how a light 404 page should look? Some screenshots in the description above of this current implementation.

@renintw renintw requested a review from ryelle November 20, 2022 20:01
@jasmussen
Copy link

Ah, thanks for the ping.

My personal take is that the fewer different 404 pages we can have, the better, i.e. it would be great if the 404 for all (most?) subpages literally were the same, there doesn't seem to be a great reason to fragment here.

However if there is a need for a light version, I would put it as close to WordPress/wporg-showcase-2022#15 in inverted form as possible. That is, the "Oops" is entirely decorative, and should be as light a gray as we can muster. There also shouldn't be any header or subnavigation. CC: @beafialho @javierarce

@bengreeley
Copy link

I agree, I'd love it if the 404 pages were as universal as possible across the website.

What are the plans for the 'Notes' and 'Call to Action' sections on the bottom? We'll want to be careful about caching any database queries on 404 pages since it's going to get so much traffic at so many different URLs. This is probably something you all are already considering, but I wanted to be sure I mentioned it. :-) I wonder if we need those sections and if we should simply match the simplicity of the News 404 page.

Thank you for your work on this!

@renintw
Copy link
Contributor Author

renintw commented Nov 23, 2022

Thanks for the feedback! @bengreeley @jasmussen

The header, subnav, and the 'Notes' and 'Call to Action' sections have been removed.
It has also been changed to the dark version, the style is based on the News 404 page.
I've updated the screenshots in the description.

@renintw
Copy link
Contributor Author

renintw commented Nov 24, 2022

@jasmussen May I ask your thought about the text in the red square in the attached img? Do you think we can get rid of the text and just change it to something like try searching sites using the field below.? This allows us not to create a new pattern in a child theme (e.g., showcase) simply for overriding the text. In News, the search only searches locally rather than all of the .org, so it seems to me that it's ok to do so.

image

@jasmussen
Copy link

For the sake of going generic, I think we could go even simpler, yes. I'd be happy just with:

Go to the homepage or try searching using the field below.

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

One suggestion, otherwise this looks good 💯

@renintw renintw merged commit ee74834 into trunk Nov 29, 2022
@renintw renintw deleted the add/404-page branch November 29, 2022 17:45
iandunn added a commit that referenced this pull request Aug 21, 2023
The first param to `get_site_url()` should be a site ID, or `null` for the current site. Passing a string can cause a `switch_to_blog( 0 )` in some circumstances. That breaks SQL queries because the table doesn't exist. The first broken query results in a `wp_die()`, but that leads to more queries which in turn call `wp_die()`.

Passing `null` fixes the error while preserving the original intent from #54 (comment).
iandunn added a commit that referenced this pull request Aug 21, 2023
The first param to `get_site_url()` should be a site ID, or `null` for the current site. Passing a string can cause a `switch_to_blog( 0 )` in some circumstances. That breaks SQL queries because the table doesn't exist. The first broken query results in a `wp_die()`, but that leads to more queries which in turn call `wp_die()`.

Passing `null` fixes the error while preserving the original intent from #54 (comment).
iandunn added a commit that referenced this pull request Aug 22, 2023
The first param to `get_site_url()` should be a site ID, or `null` for the current site. Passing a string can cause a `switch_to_blog( 0 )` in some circumstances. That breaks SQL queries because the table doesn't exist. The first broken query results in a `wp_die()`, but that leads to more queries which in turn call `wp_die()`.

Passing `null` fixes the error while preserving the original intent from #54 (comment).
DanyloKubyshkin pushed a commit to DanyloKubyshkin/wordpress-org that referenced this pull request Jul 12, 2024
The first param to `get_site_url()` should be a site ID, or `null` for the current site. Passing a string can cause a `switch_to_blog( 0 )` in some circumstances. That breaks SQL queries because the table doesn't exist. The first broken query results in a `wp_die()`, but that leads to more queries which in turn call `wp_die()`.

Passing `null` fixes the error while preserving the original intent from WordPress/wporg-parent-2021#54 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Component] Theme Templates, patterns, CSS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Layout: 404 Page
5 participants