-
Notifications
You must be signed in to change notification settings - Fork 351
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
Iotix theme updates (Beafialho playground changes) #7945
Iotix theme updates (Beafialho playground changes) #7945
Conversation
…/github.com/beafialho/themes into beafialho-playground-changes-2024-05-21T15-26-59-744Z
@beafialho I reviewed the original changes in the other PR, and included them here. Changes all look good, so from a design review this one is good to go. If you have a moment to take a quick look at this one, we can merge it! What do you think? |
Preview changesI've detected changes to the following themes in this PR: Iotix, MyMenu. 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. |
I'm not seeing the images that should be there in the home template cover and in one of the patterns below, like they are in the demo site: iotiximages.mp4 |
If it's possible to update this one manually, it would be great. Thank you! |
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.
Thanks for the ping, @jasmussen.
I noticed that we were using a mix of translation functions to translate strings in the patterns, so I've updated them all to use esc_html_e
, which is the recommended function we landed on for using in CBT. It was quicker for me to replace them all locally in my code editor, so I went ahead and committed the change: 66d94c7.
this is handled in future updates by CBT yes?
Yes that's right, I'd hope CBT will handle translations in existing PHP files (patterns) in a future release.
Otherwise, I've left some minor comments about a URL but other than that, this is looking good to me!
iotix/patterns/startup.php
Outdated
<!-- /wp:paragraph --> | ||
|
||
<!-- wp:image {"sizeSlug":"large","linkDestination":"none","className":"is-style-default"} --> | ||
<figure class="wp-block-image size-large is-style-default"><img src="<?php echo esc_url( get_stylesheet_directory_uri() ); ?>/assets/images/f24d9-column_1_image.webp" alt="" /></figure> | ||
<!-- /wp:image --> | ||
|
||
<!-- wp:paragraph {"fontSize":"medium"} --> | ||
<p class="has-medium-font-size"><a href="https://iotixdemo.wordpress.com/"><?php echo esc_html__( 'Try a demo →', 'iotix' ); ?></a></p> | ||
<p class="has-medium-font-size"><a href="https://iotixdemo.wordpress.com/"><?php esc_html_e( 'Try a demo →', 'iotix' ); ?></a></p> |
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.
Should this be using a placeholder url rather than https://iotixdemo.wordpress.com/?
iotix/patterns/startup.php
Outdated
<!-- /wp:paragraph --> | ||
|
||
<!-- wp:image {"sizeSlug":"full","linkDestination":"none"} --> | ||
<figure class="wp-block-image size-full"><img src="<?php echo esc_url( get_stylesheet_directory_uri() ); ?>/assets/images/32721-column_2_image-1.webp" alt="" /></figure> | ||
<!-- /wp:image --> | ||
|
||
<!-- wp:paragraph {"fontSize":"medium"} --> | ||
<p class="has-medium-font-size"><a href="https://iotixdemo.wordpress.com/"><?php echo esc_html__( 'See support programs →', 'iotix' ); ?></a></p> | ||
<p class="has-medium-font-size"><a href="https://iotixdemo.wordpress.com/"><?php esc_html_e( 'See support programs →', 'iotix' ); ?></a></p> |
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.
Similar here - should this be using a placeholder url rather than https://iotixdemo.wordpress.com/?
@beafialho let me know what you think of Sarah's suggestions and I'll happily update this PR so we can merge it! |
Apologies, I missed the translations in home.php and I noticed some improvements to the translations in startup.php too. Committed these changes here: 64e833b. |
Thanks, this is helpful for me to update all the older PRs that haven't used the later CBT pieces. Okay Bea when you have a moment, take a look at the playground link, see if this looks to expectations, then we'll merge! |
@beafialho no rush, but if the Playground link for this one tests well for you, we can merge this and close #7819. |
Yes, these two can use placeholder url's using All images are showing up now, the only one I'm still not seeing is the hero cover image: I also noticed a lack of side padding in two templates:
Apologies for noticing it only now 😓 but these settings seem to be changing every release and when I see the themes after a while, they usually lose the global padding they had when I created them. The code below has the changes to all. Let me know if you prefer to address these in other way that's more convenient. Single Posts This should fix that (I added 5vw to the sides of that group):
404
Search Results
|
Alright, tried to address these. It got a little fiddly, so I'll need to double check the updated Playground URL in a moment. |
Thank you, I'm seeing the cover image there now! Here's a fresh export with these latest template changes: |
@beafialho I think it's close now. Can you take a look at the demo site link, see if things are as they should be? |
Thank you @jasmussen, the changes look good to me 👍 |
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.
Yes, the home page heading text color issue I'm seeing is with the WP 6.6 rc and/or Gutenberg plugin activated. Looks related to CSS specificity(https://make.wordpress.org/core/2024/06/21/wordpress-6-6-css-specificity/). |
Pushed a fix, let me know if that works. |
@jasmussen That solves the first cover block, but not the one further down the page. Does it make sense to do this more systemically in theme.json? |
If you've a good trick for it. What can I learn of new magic here? In global styles the Heading block is styled to have the dark color, which makes sense since in most cases it will be on a white background. Right? Feel free to push to the branch in case you come up with a better fix. |
I'm heading AFK for a bit, so would appreciate you push changes directly to this branch and land it! |
I've tried a few things and I don't see a way to override the heading block text color when inside a cover block using theme.json. With all the theme.json css having equal specificity and the heading block styles being loaded after the cover block, the heading block color always wins. A workaround would be to add the following to the theme's style.css /*
* Default heading block color to white when within cover block
* to compensate for dark background colors.
*/
:root :where(.wp-block-cover .wp-block-heading:not(.has-text-color)) {
color: var(--wp--preset--color--white);
} This works in the front end and in the post editor, but not in the site editor (which seems like a bug, I'm looking into it.) |
I think the best thing to do for now is to add individual block settings to override the heading text color, like in ceb1c9d |
This reverts commit ceb1c9d.
…paragraph text color
I went ahead with a css rule to make the heading block color match the default paragraph block styles, hoping that this will be the most stable for both template and user content. Screen.Recording.2024-07-17.at.12.34.03.movI'll go ahead and land this! |
Thank you @creativecoder! |
Changes proposed in this Pull Request:
This PR replaces #7819, but fixes the conflicts.
Props @beafialho