-
Notifications
You must be signed in to change notification settings - Fork 358
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
Blissed: add theme #7932
Blissed: add theme #7932
Conversation
Preview changesI've detected changes to the following themes in this PR: Vows, Professional Business, Hola, Maywood, Alves, Loïc, Photos, Elegant Business, Shawburn, Blissed, Rockfield, Coutoire, Brompton, Inversum, Stow, Muscat, Stratford, Maverick, Exford, Morden, Seedlet, MyMenu, Mayland, Modern Business, Rivington, Sophisticated Business, Redhill, Balasana, Calm Business, Hever, Assembler, Foam, Barnsbury, LeanCV, Dalston, George Lois, Leven, Friendly Business. 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. |
General visual observations This one is very lovely. Nice work. I especially like how it uses the FULL width of the screen, no matter how big it is, it allows the photo, which is arguably the centerpiece, to shine. The top and bottom padding, as well as the way the content still scrolls with the image fixed, that's delightful. The hover style for links fails contrast checks. Can you go darker instead of lighter for the hover color? Or can it be underlined? (Don't think that's supported yet without custom CSS). No hover style is also okay. Tablet breakpoint looks great, makes good use of focal point and cropping. Mobile looks good. Super nitpick, not a blocker, I wonder if we should rename the main image to something like "couple-photo.jpg". Also, worth updating the midjourney credit to the new format that includes the CC0. It reads "All images generated by Midjourney AI platform." Template review There are quite a lot of files in the "Patterns" folder, that looks like the should be templates. Can you check and see what's up there? Style variations Style variations look great. I miss one that's more white in the background, but not a blocker. Summary This is a nice one. Well done. There's a question on those pattern/template files, which has me slightly confused. But other than that, this one will be well received! |
Thank you for your review, @jasmussen. On this one, I have:
|
Thanks for keeping at it. A few things still. I'm seeing this in the README:
What's the "Bliss ()" is that the right name? If yes, all good.
This could still be updated to include the CC0 phrasing. I'm also still seeing a slew of files inside the patterns folder: This could be my lack of understanding of the theme structure, but those look like template files. Should they be there? |
@jasmussen I don't know where these patterns come from, but once I delete them, the related pages are gone. Just as if they were templates. That's why I left them there. |
Alright, thanks for checking. I'm still curious, this seems like something that would be good to figure out, especially if the best practice is to copy one theme when building out the next. So I'll send out some feelers to see if we can get someone to gut-check this! |
The php patterns are currently the only way to include translated strings in templates for block themes. If you check the option to localize strings when saving changes to the theme, create-block-theme will move the template content into patterns and then update the templates to reference those patterns. We're hoping to be able to translate directly in html templates without this workaround in the future. The Bits proposal may be a way to do this. |
Alright, thanks for clarifying. I think it's important we remember this as a hack, both for theme reviews like this, and for future CBT updates, so we can undo this as soon as technically possible. It doesn't seem a best practice that CBT should do for other consumers of this, unless absolutely necessary (which seems to be the case at the moment.) |
@jasmussen I put back the deleted patterns from a previous version. |
Nice, thank you! Okay, give it a few minutes to let the Playground link catch up, then visit this link and click through all the templates. If the site still looks right to you, let's merge it as fully reviewed. |
Blissed is a theme created to serve as a comprehensive hub for all your wedding-related links. Designed with joy, the theme offers a sleek and intuitive interface that functions like a link tree for engaged couples to organize their event and share their thoughts with their guests.
Demo site