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.

Sign up for GitHub

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

Jump to bottom

The Shore: Added Site Title Block to Headers #8188

Merged
merged 2 commits into from
Sep 24, 2024
Merged

The Shore: Added Site Title Block to Headers #8188

merged 2 commits into from
Sep 24, 2024

Conversation

iamtakashi
Copy link
Contributor

Added Site Title Block to Headers. Fixes #8168.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Sep 19, 2024 •
edited
Loading

Preview changes

I've detected changes to the following themes in this PR: The Shore.
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.

@github-actions GitHub Actions
Copy link
Contributor

Theme-Check results

the-shore: No changes required ✅.

💡 RECOMMENDED (2)
  • No reference to register_block_pattern was found in the theme. Theme authors are encouraged to implement custom block patterns as a transition to block themes.
  • No reference to register_block_style was found in the theme. Theme authors are encouraged to implement new block styles as a transition to block themes.
ℹ️ INFO (1)
  • Only one text-domain is being used in this theme. Make sure it matches the theme's slug correctly so that the theme will be compatible with WordPress.org language packs. The domain found is the-shore.

alaczek
alaczek reviewed Sep 20, 2024
View reviewed changes
the-shore/patterns/header-homepage.php
@@ -15,9 +15,9 @@
<!-- wp:group {"align":"full","layout":{"type":"default"}} -->
<div class="wp-block-group alignfull"><!-- wp:group {"align":"full","style":{"spacing":{"padding":{"top":"var:preset|spacing|40","bottom":"var:preset|spacing|40","left":"5vw","right":"5vw"}}},"layout":{"type":"flex","justifyContent":"space-between","verticalAlignment":"center"}} -->
<div class="wp-block-group alignfull" style="padding-top:var(--wp--preset--spacing--40);padding-right:5vw;padding-bottom:var(--wp--preset--spacing--40);padding-left:5vw"><!-- wp:group {"layout":{"type":"flex"}} -->
<div class="wp-block-group"><!-- wp:image {"width":"200px","sizeSlug":"full","linkDestination":"none"} -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we keep the "width":"200px" param? Without it the logo is tiny.

@iamtakashi
Copy link
Contributor Author

iamtakashi commented Sep 20, 2024 •
edited
Loading

Should we keep the "width":"200px" param? Without it the logo is tiny.

I'm not sure. I kept it initially, but it felt too large when the logo was 1:1. I removed it and left it as default because it depends on the aspect ratio of the logo, and the site admin is expected to adjust the size accordingly.

localhost local_ (16)

@alaczek
Copy link
Contributor

alaczek commented Sep 24, 2024

and the site admin is expected to adjust the size accordingly.

Fair enough, I'll make a note about this on the showcase page.

@alaczek alaczek merged commit 247cb45 into trunk Sep 24, 2024
3 checks passed
@alaczek alaczek deleted the add/the-shore-site-title branch September 24, 2024 02:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alaczek alaczek alaczek left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The Shore: Header missing site title
2 participants
@iamtakashi @alaczek