-
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
Poetry: add alternative header part #179
Poetry: add alternative header part #179
Conversation
poetry/parts/header-small.html
Outdated
@@ -0,0 +1,25 @@ | |||
<!-- wp:group {"style":{"spacing":{"padding":{"top":"32px","bottom":"64px"}}},"className":"header-small","layout":{"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.
Instead of pixel values, can we use any of the presets (small, medium, large...)? If none work, let's use rem values instead
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 just checked and the old header has the same problem, we should do the change on both to keep it consistent
poetry/parts/header-small.html
Outdated
<!-- /wp:group --> | ||
|
||
<!-- wp:image {"width":"auto","height":"64px","sizeSlug":"large","linkDestination":"none"} --> | ||
<figure class="wp-block-image size-large is-resized"><img src="http://wp-community-day.local/wp-content/themes/poetry/assets/images/feather.png" alt="" style="width:auto;height:64px"/></figure> |
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 url won't work on other people's sites, let's do something like this
@MaggieCabrera Thanks for your review. I will try to address your comments here regarding the use of pixel values. I will add the spacingSizes in theme.json as in twentytwentyfour. And I will also convert the header-small template to be a pattern instead. But I just notice that the page navigation is missing so maybe I will have to come up with a different solution for the small version of the header that can accomodate the navigation. There's no navigation in the original header pattern also. Or maybe It's supposed to be like that and this specific theme don't need a navigation in the header. What do you think? |
I think we can make of it what we like. It makes sense to me that the focus of this kind of blog would be the post content, but if you click on internal pages you would like to go back to the home. Maybe we want our smaller header to just have the logo and the page title, removing the text "A collection of valuable thoughts...". In fact, now that I look at it, we should probably replace the image block for the site logo block and the other paragraph that we have for the "tagline" block instead. |
Preview changesI've detected changes to the following themes in this PR: Poetry. You can preview these changes by following the links below: I will update this comment with the latest preview links as you push more changes to this PR. |
@MaggieCabrera I'm so sorry to have abandoned this PR. Right after I had open it I got sick. And then, got to focus on work for some time. And then, came holidays and I totally forgot to get back here. I see you are trying to merge this into another PR. Tell me if I can help in some way. I will also check the current open issues and see if I can help with any. |
Hi! so happy to see you back. I've been busy myself, so don't worry. I think both PRs are a little stale, so if you want to pick up the work it should be fine. Maybe from a fresh PR and we can close the other 2? |
Closing in favor of #235 |
Closes #174