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

Major update to Hall theme #7688

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Major update to Hall theme #7688

wants to merge 1 commit into from

Conversation

alaczek
Copy link
Contributor

@alaczek alaczek commented Mar 19, 2024

This is a major update to Hall theme (not yet launched).

(Previous PR: #7319)

This is a major update to Hall theme (not yet launched)
hall/parts/exhibition-block.html Show resolved Hide resolved
@@ -2,13 +2,13 @@
<div class="wp-block-group alignwide"><!-- wp:group {"align":"wide","layout":{"type":"constrained"}} -->
<div class="wp-block-group alignwide"><!-- wp:columns {"align":"wide","style":{"spacing":{"blockGap":{"left":"1.5rem"}}}} -->
<div class="wp-block-columns alignwide"><!-- wp:column {"verticalAlignment":"center"} -->
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:paragraph {"align":"left","style":{"spacing":{"blockGap":{"left":"1.5rem"}},"typography":{"textTransform":"uppercase"}}} -->
<p class="has-text-align-left" style="text-transform:uppercase">Designed with <a rel="nofollow" href="https://wordpress.org">WordPress</a></p>
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:paragraph {"align":"left","style":{"spacing":{"blockGap":{"left":"1.5rem"}}}} -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The theme credit should be turned into a pattern and wrapped in a translation function, like this: https://github.com/Automattic/themes/blob/trunk/allez/patterns/footer.php

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I understand it, template parts and patterns work the same here. In fact, TPs are Patterns on the Editor (screenshot). Create Block Theme doesn't export our custom sets as Header or Footer as intended but there they are, in General.

Screenshot 2024-03-25 at 09 53 11

Copy link
Collaborator

Choose a reason for hiding this comment

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

To offer context, Allez, the theme you mentioned, was reviewed by a developer (Jeff Ong) before launch. That is why these issues are covered on that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand it, template parts and patterns work the same here.

This is true. The reason we always convert this bit to a pattern is that it contains a string that needs to be translatable, and that requires making it a pattern.

<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column {"verticalAlignment":"center"} -->
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:navigation {"overlayMenu":"never","layout":{"type":"flex","justifyContent":"left"},"style":{"spacing":{"blockGap":"2rem"}}} /--></div>
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:navigation {"ref":34,"overlayMenu":"never","layout":{"type":"flex","justifyContent":"left"},"style":{"spacing":{"blockGap":"2rem"}}} /--></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ref attribute should be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

<!-- /wp:paragraph --></div>
<!-- /wp:column -->

<!-- wp:column {"verticalAlignment":"center"} -->
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:navigation {"overlayMenu":"never","style":{"typography":{"fontStyle":"normal","fontWeight":"600","textTransform":"uppercase"}},"fontSize":"small"} /--></div>
<div class="wp-block-column is-vertically-aligned-center"><!-- wp:navigation {"ref":34,"overlayMenu":"never","layout":{"type":"flex","justifyContent":"left"},"style":{"spacing":{"blockGap":"2rem"}}} /--></div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ref attribute should be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same answer.

@@ -4,31 +4,13 @@
* Slug: hall/404
* Inserter: no
*/
declare( strict_types = 1 );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need to keep this bit of code in all PHP files.

<!-- /wp:heading -->

<!-- wp:paragraph -->
<p><?php echo esc_html__( 'Founded in the heart of Buenos Aires in 1896, the International Art Museum has, from its inception, housed a rich diversity of works spanning different periods and artistic movements globally. It emerged with the mission not only to safeguard the richness of international art but also to foster the flourishing of Argentine art within a context of cultural renewal.', 'hall' ); ?></p>
<!-- wp:paragraph {"style":{"typography":{"lineHeight":1.4}}} -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the strings in this file need to be wrapped in a translation function.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know how to do that. I work on the Editor, with quite limited code skills. But I can learn some best practices to help if I can.

hall/readme.txt Show resolved Hide resolved
<!-- /wp:heading -->

<!-- wp:paragraph -->
<p>It looks like nothing was found at this location.<br>Maybe try a search?</p>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The 404.php pattern needs to be inserted here instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same answer.

@@ -1 +1,195 @@
<!-- wp:pattern {"slug":"hall/home"} /-->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the old version was right - this file should include the home.php pattern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need to figure out how to fix this. I use only the Editor, create everything and export with the Create Block Theme plugin. Please let me know what to do to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the CBT plugin doesn't cover all things required to make a theme launch-ready (on .com or .org).
This issue should take care of, but until then it's manual work I'm afraid.

For now, once I export the theme with CBT I go through the files and look for these known issues. You'll notice that it's the same things coming up over and over again - mostly issues around content that needs to be translatable and thus requires turning it into a pattern, removing the ref from navigation, making sure that all assets were properly included and referenced in the files, etc.

hall/templates/page.html Show resolved Hide resolved
@henriqueiamarino
Copy link
Collaborator

henriqueiamarino commented Mar 25, 2024

Hey @alaczek, here's a new/final zip file with the updated href links.
hall.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants