search

Automattic / themes

Free WordPress themes made by Automattic for WordPress.org and WordPress.com.
https://themeshaper.com
GNU General Public License v2.0
844 stars 342 forks source link

Vows: add theme #7865

Closed henriqueiamarino closed 6 hours ago

henriqueiamarino commented 2 weeks ago

Vows works as a comprehensive hub for all your wedding-related links. Inspired by the timeless beauty of wedding stationery and photography, this theme provides a sleek and intuitive interface that acts like a link-in-bio solution for engaged couples. Demo site

screenshot

github-actions[bot] commented 2 weeks ago

Preview changes

I've detected changes to the following themes in this PR: Russell, Vows, Sunderland, Alves, Farrow, Loïc, Otis, Meraki, Vitrum, Artly, Hall, Attar, Dorna, Appleton, Heiwa, Marl, Varia, Erma.

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.

jasmussen commented 2 days ago

General visual observations

Lovely theme. I understand this is at the "close to launch" technical review state, so if it has to move forward, visually it's in a good place to do so. But I can't help myself, I'll always see details and comment on them. But hopefully this feedback can be applied to future themes in the pipeline, in more generic ways, even it they might not apply to this one specifically. Hope that makes sense!

In this case, there are a couple of nice weddingy details I like. The flourish here:

Screenshot 2024-07-01 at 12 46 00

The "Save the date" colors

Screenshot 2024-07-01 at 12 46 02 —though perhaps that could also benefit from left alignment and a grid style, perhaps?

The typography is legible, and the serif (with the ampersand mentioned) works well. I do wonder if the body text couldn't benefit from a slightly lighter weight. A lighter weight for the heading fonts as well might also benefit. Perhaps even slightly smaller font size might give some elegance, and afford a bit more space between subheadings and text before/after in cases like this:

Screenshot 2024-07-01 at 12 47 31

The lighter weight is perhaps the main bit of feedback. The colors feel slightly dark for wedding concept. The beige works, but the blue feels a bit cool. Another angle could be a black and white style variation, but with lots of white?

It tracks for the theme, it's sort of a microsite, that there's not that much difference in layout across pages. With such a simple layout, instead of additional templates, it could be a fun opportunity to go a bit more broad with some of the style variations, some that featured larger type, some with smaller, some with lots of white, some with golden colors, etc. It feels like a great use of duotone, so there are some opportunities there. The style variations you provided work well and show this potential, but the duotone could potentially match the overall spot color, and in general the colors feel still a bit dark and cool rather than light and warm.

I wouldn't call any of this blocking feedback. If you can/want to take a stab at tweaks, go for it, otherwise it can be some general thoughts that can hopefully inspire future themes! 🙏

Mobile works very well 👌

File and readme reviews

Readme looks good, I like the description.

The AI section could use a small update to the new boilerplate we discussed in another thread, it still says the old credit: "All images used in the templates were generated with AI using Midjourney."

I'm seeing some strange font filenames: Screenshot 2024-07-01 at 12 57 44

I don't think that's necessarily problematic, but just for curiosity, is that how CBT names them?

One issue that needs to be addressed, I'm seeing some instances of "localhost" in the theme.json file:

"src": "http://localhost.local/wp-content/uploads/fonts/Reforma1918-GrisItalica.otf"

Not sure if CBT should be handling those or not. Let me know if this is on your radar, otherwise I can ping a dev for input.

Summary

Nice theme! Some opportunities for lighter/warmer colors and a bit more whitespace, but fine to launch. The readme needs a small update, and there's a question on the localhost in the theme.json file that we need to figure out first.

henriqueiamarino commented 2 days ago

Thanks for the overall feedback.

Regarding the weird font naming and the localhost link: I don't know why it occurs, but yes, CBT has been naming fonts like that and has failed to localize fonts and images. That is why I always have to check and change them manually—which I will do on this one.

jasmussen commented 2 days ago

Thanks. A quick CC to @matiasbenedetto or @mikachan in case we're doing something wrong here.

henriqueiamarino commented 2 days ago

It has happened constantly lately, @matiasbenedetto, not just with these fonts. I generally fix all of them, but I forgot to do it on this one.

mikachan commented 2 days ago

It has happened constantly lately,

I'm not able to reproduce this, all my font files are relative to the theme folder, e.g.:

{
                    "fontFace": [
                        {
                            "fontFamily": "Piazzolla",
                            "fontStyle": "normal",
                            "fontWeight": "100",
                            "src": [
                                "file:./assets/fonts/N0b52SlTPu5rIkWIZjVKKtYtfxYqZ4RJBFzFfYUjkSDdlqZgy7LYx3L31AHfAAy5.woff2"
                            ]
                        }
                    ],
                    "fontFamily": "Piazzolla, serif",
                    "name": "Piazzolla",
                    "slug": "piazzolla"
                }

Could you confirm which tools and versions you're using? My test above was using Local as my test environment, with WP 6.6 RC1, GB deactivated, and CBT 2.2.0. I'll keep testing to see if I can reproduce.

matiasbenedetto commented 2 days ago

I could reproduce it. Seems like a bug or at least a blind spot of the 'clone theme' functionality of CBT.

To reproduce the issue you can do the following:

  1. Install a font face
  2. Clone the theme using these buttons: Create Theme -> Clone Theme.
  3. Save using Save Changes to the Theme button.

I'll create an issue in the CBT repo about this.

mikachan commented 2 days ago

Ohh I see, thanks @matiasbenedetto! I only tried this using the "Save Fonts" option.

henriqueiamarino commented 2 days ago

Thanks, @matiasbenedetto! Now that I am adding themes to Dotorg, I must constantly clone and submit themes under new names. So, it has been impacting my work a lot.

matiasbenedetto commented 2 days ago

Submitted the issue to the CBT repo: https://github.com/WordPress/create-block-theme/issues/677

henriqueiamarino commented 6 hours ago

I am closing this PR as I commit unwanted changes with the ones from Vows. I've cloned the Trunk three times today already, but every time I create a PR, there's a risk of it being outdated. I don't know yet how to fix this, so I'll create a new PR.