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

Update theme goodskin #8016

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

beafialho
Copy link
Collaborator

@beafialho
Copy link
Collaborator Author

After submitting Goodskin to WordPress.org, I got a number of comments, some of which I was able to address. The others are listed below.

Warnings
WARNING: Found ="<?php esc_html_e in patterns/home.php. Use esc_attr_e() inside HTML attributes, and esc_url() for link attributes. A manual review is needed.
WARNING: Found ="<?php esc_html_e in patterns/two-column-gallery.php. Use esc_attr_e() inside HTML attributes, and esc_url() for link attributes. A manual review is needed.

Skip Links

Check skip links requirement here. https://make.wordpress.org/themes/handbook/review/required/#skip-links
Recommended reading, https://make.wordpress.org/themes/2019/07/14/how-to-add-and-test-skip-links/
It is hiding the first post. Check it properly.

Extras

Broken block found in the editor, https://i.rankmath.com/i/3sz1z4
Check theme.json error, https://prnt.sc/9NDDpv44e0KC

Translation
Translation missing on about-page-heading-paragraph-image.php file. Check other patterns file too.

Copy link
Contributor

github-actions bot commented Aug 14, 2024

Preview changes

I've detected changes to the following themes in this PR: Goodskin.

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.

@jasmussen
Copy link
Member

I'm curious why this PR is already out of date with trunk, and needs a rebase. Bea did you pull latest changes before making this update?

As for the changes in particular, I'll unfortunately have to ask for help from @vcanales, @madhusudhand, @mikachan or @matiasbenedetto. Did Bea use CBT wrong here, as far as the templates that were put in the patterns directory? Or is there a CBT fix to make?

@beafialho
Copy link
Collaborator Author

Bea did you pull latest changes before making this update?

I do not use the GH desktop app. What I did was download the latest version of the theme, add it to WP Playground, save my changes and export to a new PR 🤔

@vcanales
Copy link
Contributor

Did Bea use CBT wrong here, as far as the templates that were put in the patterns directory? Or is there a CBT fix to make?

I do not use the GH desktop app. What I did was download the latest version of the theme, add it to WP Playground, save my changes and export to a new PR 🤔

The best way to ensure that PRs for previews work well is to get the latest code from GitHub, because we could have made changes to a theme without deploying them, which is what I think happened in this PR.

@beafialho would it be possible to create another PR following a workflow where the changes you make to the theme are made to the latest code stored GitHub? I'm asking because the conflicts we see here make it hard to determine what to keep. If you need any help figuring out a workflow that will make it easier to review your themes, please reach out to me here on in private!

Copy link
Member

@madhusudhand madhusudhand left a comment

Choose a reason for hiding this comment

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

Since the theme already in trunk, it should be showing differences of files such as style.css, theme.json

But it shows them as new changes.

@beafialho can you try merging latest trunk into this branch?

goodskin/theme.json Outdated Show resolved Hide resolved
goodskin/theme.json Outdated Show resolved Hide resolved
goodskin/style.css Outdated Show resolved Hide resolved
@madhusudhand madhusudhand force-pushed the playground-changes-2024-08-14T11-48-06-121Z branch from faf8640 to 2888040 Compare August 30, 2024 11:20
@madhusudhand
Copy link
Member

@jasmussen can you suggest on the following review comment from dotOrg?

Images created from Modjourney is not GPL compatible too and we won't allow to use fully AI generated images. Check meeting notes here, https://make.wordpress.org/themes/2023/08/10/themes-team-meeting-notes-august-08-2023/

@madhusudhand
Copy link
Member

Since the theme already in trunk, it should be showing differences of files such as style.css, theme.json

But it shows them as new changes.

@beafialho can you try merging latest trunk into this branch?

Resolved conflicts and addressed dotOrg feedback.

@jasmussen
Copy link
Member

Thanks so much. I'll need to ask some folks about the Midjourney issue.

@alaczek
Copy link
Contributor

alaczek commented Sep 6, 2024

Thanks so much. I'll need to ask some folks about the Midjourney issue.

Thanks for looking into this @jasmussen . Please share the outcome on a p2 or somewhere with broader visibility!

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

Successfully merging this pull request may close these issues.

5 participants