-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add 404 page #54
Conversation
source/wp-content/themes/wporg-parent-2021/patterns/404-page-subtitle.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-parent-2021/patterns/404-page.php
Outdated
Show resolved
Hide resolved
source/wp-content/themes/wporg-parent-2021/patterns/404-page.php
Outdated
Show resolved
Hide resolved
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. |
@jasmussen Can you chime in with how a light 404 page should look? Some screenshots in the description above of this current implementation. |
813ef34
to
8081bd9
Compare
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 |
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! |
2a23ea5
to
58b7bce
Compare
Thanks for the feedback! @bengreeley @jasmussen The header, subnav, and the 'Notes' and 'Call to Action' sections have been removed. |
@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 |
For the sake of going generic, I think we could go even simpler, yes. I'd be happy just with:
|
There was a problem hiding this 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 💯
source/wp-content/themes/wporg-parent-2021/patterns/404-page.php
Outdated
Show resolved
Hide resolved
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).
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).
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).
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).
Fixes WordPress/wporg-showcase-2022#15
Screenshots
How to test the changes in this Pull Request: