-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adding Poetry: 404 and search templates #145
base: trunk
Are you sure you want to change the base?
Conversation
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.
This is looking nice! I added a few comments. I think we can follow up on the issue with the date on a separate PR.
poetry/templates/404.html
Outdated
<div class="wp-block-group"> | ||
<!-- wp:group {"style":{"spacing":{"blockGap":"88px"}},"layout":{"type":"flex","orientation":"vertical"}} --> | ||
<div class="wp-block-group"> | ||
<!-- wp:site-title {"level":0,"isLink":false,"style":{"typography":{"fontSize":"24px","fontStyle":"normal","fontWeight":"600"}},"textColor":"custom-text"} /--> |
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.
I think it would be better to keep the header element like the index.html template has it. It's more consistent between pages and also we need to keep the header
element for accessibility reasons. This comment goes for both templates
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.
Ok, I forgot the header in poetry was super huge. My recommendation still remains, let's make a secondary header pattern or template part to reuse in internal pages that doesn't have the huge text, but let's keep it consistent between pages. And let's use the tagName header on it so we don't miss the header element
</main> | ||
<!-- /wp:group --> | ||
|
||
<!-- wp:template-part {"slug":"footer","theme":"poetry","tagName":"footer","area":"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.
Both templates should have a footer element. We can go without the area attribute (the theme.json file already includes this info) and the theme slug (it will make the template part break if the theme is installed in a subfolder)
|
||
<!-- wp:group {"tagName":"main","backgroundColor":"custom-background-light","layout":{"contentSize":null,"type":"constrained"}} --> | ||
<main class="wp-block-group has-custom-background-light-background-color has-background"> | ||
<!-- wp:query {"queryId":0,"query":{"perPage":10,"pages":0,"offset":0,"postType":"post","order":"desc","orderBy":"date","author":"","search":"","exclude":[],"sticky":"","inherit":true,"taxQuery":null,"parents":[]},"tagName":"main","layout":{"contentSize":null,"type":"constrained"}} --> |
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.
Since this Query block should look the same as the one in the index template, what about we make it into a pattern instead and then we avoid duplicating code over both templates? (that way when we fix the problem with the date we only change it in one place)
First part of this commit - addressing feedback to keep the header element consistent between the 404 template and the index template for better accessibility.
@samtoohey93 be careful here because this needs a rebase |
Ah damn sorry I was doing this rushing out the door this arvo 🤦♂️ |
#130
404
I gave it a poetry-esque 404 message.
Search
The loop results uses the same style as the index page, but looks like this needs some adjustment to the column/group widths to stop the date wrapping?