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

Layover: add theme #7890

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Layover: add theme #7890

merged 4 commits into from
Aug 13, 2024

Conversation

henriqueiamarino
Copy link
Collaborator

@henriqueiamarino henriqueiamarino commented Jun 21, 2024

Layover displays large titles and excerpts that scroll over an image on the Homepage and neat single pages for users who want their blogging to be simple.
Demo site

screenshot

Copy link
Contributor

github-actions bot commented Jun 21, 2024

Preview changes

I've detected changes to the following themes in this PR: Alves, Appleton, Artly, Assembler, Balasana, Barnsbury, Bitácora, Brompton, Calm Business, Canard, Cookbook, Course, Coutoire, Dalston, Dara, Elegant Business, Erma, Eventual, Exford, Foam, Fontaine, Friendly Business, George Lois, Hall, Heiwa, Hever, Iotix, Jaida, Layover, Leven, Loïc, Mayland, Maywood, Meraki, Modern Business, Morden, Muscat, Otis, Pendant, Photos, Professional Business, Redhill, Rivington, Rockfield, Seedlet, Shawburn, Sophisticated Business, Stow, Stratford, Sunderland, Upsidedown, Varia, Vitrum.

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.

⚠️ Note: Child themes are dependent on their parent themes. You will have to install the parent theme as well for the preview to work correctly.

@henriqueiamarino
Copy link
Collaborator Author

The background-image inside the Cover in Home is not showing, and I can't fix it.
I ask the reviewer to check it, please.

@jasmussen
Copy link
Member

General visual observations

This is a favorite of mine, looks fantastic.

It's a bit hard to review fully, though, as I'm also not seeing the background image you're referring to. But I'm not actually seeing any Cover block at all in the Home template:

Screenshot 2024-08-07 at 10 44 05

Feel free to hit me up in chat and we can review sync if need be, maybe screenshare, see where it might be missing or what I should check.

Other than that, virtually everything looks great. Good on mobile, no glitches, templates nice.

Style variations are a bit bare. The muted colors are nice, but I'm missing both some font alternatives, and perhaps a light/dark monochrome styles. Not blockers, mainly food for thought.

I did notice one awkwardly named font:

Screenshot 2024-08-07 at 10 46 21

In fact, there are quite a lot of fonts in the font folder, but in testing the templates, it looks like only a single sans-serif font is used. Maybe you did add fonts for the style variations, but they're not working fully as intended? Something to look at.

File and readme reviews

Readme looks good.

Summary

Great theme, will be a nice one to land. We need to fix the missing image issue, and figure out what's up with the fonts. Other than that, this one is close.

@henriqueiamarino
Copy link
Collaborator Author

Thanks, @jasmussen. I fixed as much as I could and improved the style variations. I need help fixing the home template, though. CBT plugin replaces the image file with a URL from the demo site and fails to load it.

@@ -6,8 +6,8 @@
* Inserter: no
*/
?>
<!-- wp:cover {"url":"https://localhost.local/wp-content/uploads/2024/08/layover_background.jpg","hasParallax":true,"dimRatio":0,"customOverlayColor":"#212121","isUserOverlayColor":true,"contentPosition":"center center","style":{"spacing":{"margin":{"top":"0rem","bottom":"0rem"},"padding":{"top":"0rem","right":"0rem","bottom":"0rem","left":"0rem"}}},"layout":{"type":"constrained"}} -->
<div class="wp-block-cover has-parallax" style="margin-top:0rem;margin-bottom:0rem;padding-top:0rem;padding-right:0rem;padding-bottom:0rem;padding-left:0rem"><span aria-hidden="true" class="wp-block-cover__background has-background-dim-0 has-background-dim" style="background-color:#212121"></span><div class="wp-block-cover__image-background has-parallax" style="background-position:50% 50%;background-image:url(https://localhost.local/wp-content/uploads/2024/08/layover_background.jpg)"></div><div class="wp-block-cover__inner-container"><!-- wp:template-part {"slug":"header","align":"wide"} /-->
<!-- wp:cover {"url":"<?php echo esc_url( get_stylesheet_directory_uri() ); ?>/assets/images/layover_background.jpg","hasParallax":true,"dimRatio":0,"customOverlayColor":"#212121","isUserOverlayColor":true,"contentPosition":"center center","style":{"spacing":{"margin":{"top":"0rem","bottom":"0rem"},"padding":{"top":"0rem","right":"0rem","bottom":"0rem","left":"0rem"}}},"layout":{"type":"constrained"}} -->
Copy link
Member

Choose a reason for hiding this comment

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

I pushed a change manually to fix this issue. Not sure why CBT didn't fix this: did we do something wrong in the export process, @matiasbenedetto? The image landed in the right directory, it just wasn't attached here.

@jasmussen
Copy link
Member

With the latest fix, I think this is ready to merge! Nice work.

@henriqueiamarino henriqueiamarino merged commit 4060e20 into trunk Aug 13, 2024
1 check passed
@henriqueiamarino henriqueiamarino deleted the add/layover branch August 13, 2024 10:15
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.

3 participants