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

Poetry: add alternative header part #179

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

adrianaleites
Copy link
Contributor

@adrianaleites adrianaleites commented May 17, 2024

  • Added a smaller alternative header for pages like 404 and search.
  • Added functions.php to enqueue styles.

Closes #174

@@ -0,0 +1,25 @@
<!-- wp:group {"style":{"spacing":{"padding":{"top":"32px","bottom":"64px"}}},"className":"header-small","layout":{"type":"constrained"}} -->
Copy link
Collaborator

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

Copy link
Collaborator

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

<!-- /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>
Copy link
Collaborator

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

@adrianaleites adrianaleites changed the title Poetry: add alternative header part (#174) Poetry: add alternative header part May 21, 2024
@adrianaleites
Copy link
Contributor Author

@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?

@MaggieCabrera
Copy link
Collaborator

@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.

@iamsam2e iamsam2e added question Further information is requested discussion Theme: Poetry labels Jun 9, 2024
Copy link

Preview changes

I'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.

⚠️ Note: The preview sites are created using WordPress Playground. You can add content, edit settings, and test the themes as you would on a real site, but please note that changes are not saved between sessions.

@adrianaleites
Copy link
Contributor Author

@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.

@MaggieCabrera
Copy link
Collaborator

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion question Further information is requested Theme: Poetry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Poetry: Create alternative header
3 participants