-
Notifications
You must be signed in to change notification settings - Fork 113
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
This comment was marked as resolved.
This comment was marked as resolved.
@cliffralessio Thanks for the PR. Could you please remove this class |
patterns/hidden-404.php
@cliffralessio Thanks for making progress in resolving this issue. |
This comment was marked as resolved.
This comment was marked as resolved.
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.
Great progress @cliffralessio, thanks for working on this!
There are some presets already merged that you can make use of. I left some comments about a few things that are repeated throughout the code. If you re-base the branch with the latest changes in trunk you'll have the chance to use the linter and catch many of the issues that are currently happening.
The other thing that I'd like to ask is if you can please use tabs instead of spaces for the blank spaces, so that we're consistent with that across default themes.
@juanfra Thank you for helping me, this is my first contribution and I know I am making many mistakes, but on the other hand, thanks to you and @carolinan I am learning a lot! |
Could we add a meaningful 404-related image name instead of this |
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.
Thank you for working on this @cliffralessio - There has been a lot of progress! the code looks way better, but the design is not there quite yet. When checking your branch I can see that the content area is not taking the full width and it's shown in a narrow column.
I'm sharing a screencast showing how you can adjust some of the configurations of the different blocks you have to get closer. Please let me know if I can be of help; we're almost there 🏅
404-template.mov
@juanfra Thank you for your help, I followed your instructions but I am not sure about one thing. When I see my template from a 1440px device everything is fine but, when I see it from a larger device my tamplate is always on the left of the screen. Is everything okay? |
templates/404.html
Outdated
<!-- /wp:group --> | ||
|
||
<!-- wp:template-part {"slug":"footer","area":"footer","tagName":"footer"} /--> | ||
|
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.
Please change this file to using tabs, not spaces, and remove the trailing space at the end.
The last line of the file should be empty.
Even on smaller screens, the horizontal spacing is too large. It is getting the left and right spacing from both the template and the pattern. Try removing the left and right padding from the columns block. The block gap between the two columns could be larger to better match the design. |
This comment was marked as resolved.
This comment was marked as resolved.
Thank you for your work @cliffralessio! I gave it a go with the row block and was able to achieve the design. But when I test it on mobile it doesn't look good. Somehow the same is happening with the grid block. 404.mp4Here's the markup in case there's something wrong with what I'm doing:
|
This comment was marked as resolved.
This comment was marked as resolved.
patterns/hidden-404.php
Outdated
<div class="wp-block-columns alignwide" style="padding-top:var(--wp--preset--spacing--50);padding-bottom:var(--wp--preset--spacing--50)"> | ||
<!-- wp:column {"width":"40%"} --> | ||
<div class="wp-block-column" style="flex-basis:40%"> | ||
<!-- wp:image {"sizeSlug":"large","linkDestination":"none","className":"is-resized"} --> |
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.
<!-- wp:image {"sizeSlug":"large","linkDestination":"none","className":"is-resized"} --> | |
<!-- wp:image {"sizeSlug":"large"} --> |
Image is not resized and linked.
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.
These are the default values when you add an image in the block editor.
If they are removed, the editor has to manually add the class name and change the markup to this:
<!-- wp:image {"id":19,"className":"size-large"} -->
Please motivate why it should be removed?
@beafialho I think it doesn't work because you are using a fixed size for the width: |
If this design is blocked, should we try a different design? |
Why is it blocked? |
I am reading comments from two people who are unable to make the design work well on all screen sizes. |
If it doesn't work with the row, nor the grid block, I tried with columns block. It achieves the design and it works across screen sizes 🎉 I believe this should solve it: 404.mp4
|
The following changes needs to be made:
The opening and closing of the main element must be in the HTML file: But without the meta data, which you must remove:
Meaning the columns block must be in the PHP file, replacing the current markup. In the columns, the source of the image must be updated, because when copy-pasting the pattern, the previous PHP code is lost. |
<!-- wp:group {"style":{"layout":{"columnSpan":7,"rowSpan":1}},"layout":{"type":"default"}} --> | ||
<div class="wp-block-group"> | ||
<!-- wp:heading {"level":3,"style":{"typography":{"fontStyle":"normal","fontWeight":"500"}}} --> | ||
<h3 class="wp-block-heading" style="font-style:normal;font-weight:500"> |
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.
The main heading on the page should be an H1.
I am not able to do a git pull for this PR, it is getting rejected.
For the sake of speed, I'll try to create a new PR and merge it asap. |
Description
<-- This pull request introduces a new 404 error page pattern for the Twenty Twenty-Five WordPress theme. -->
Image License and Source
<-- Image License: Pexels License-->
<-- Image Source: https://www.pexels.com/-->
Pexels License Summary
<-- The images on Pexels can be used for free. -->
<-- Commercial and non-commercial use is allowed. -->
<-- Attribution is not required, but giving credit to the photographer is appreciated. -->
Co-authored-by: cliffralessio [email protected]