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

Extracted any logic that may need to be tested from the api class #522

Merged
merged 28 commits into from Mar 27, 2024

Conversation

pbking
Copy link
Contributor

@pbking pbking commented Mar 19, 2024

Fixes: #162

This change moves any logic that may need to be tested outside of the api implementation. This logic is now in a 'Theme_Createclass. Logic is re-used in the otherTheme-*` classes whenever possible.

Additionally this change incorporates the removal of site-specific attributes from template markup fixing #162. That work began in #524 and was copied to this branch.

This also fixes #336 since the asset processing from the API was cleaned up and now the assets are being copied as expected. The logic to process this is now quite a bit simpler: the block parsing is mostly gone, with only the gathering of URLs happening as it used to. Once that happens the URLs that were gathered are transformed to a local path, wrapped in a PHP tag and replaced in the template.

This fixes #489 as it escapes (most) strings during template parsing.

Many comments were added to clarify the code and functions renamed where appropriate.

Lastly templates that have strings (currently only in paragraphs, additional blocks are needed) have those strings wrapped in PHP tags and localized. (Any templates with PHP tags are converted to patterns).

To Test

Complete the following actions to ensure that the resulting theme matches expected results:

  • Make changes to Global Styles and save the theme (changes should be reflected in the theme.json file)
  • Make changes to templates and save the theme
    • Image assets should be included in the theme, referenced from the pattern, which is referenced from the template
    • Paragraph blocks should be localized in a pattern and referenced from a template
    • templates with NO images or paragraphs shouldn't be converted to a pattern
  • Export a saved theme
  • Clone a theme
  • Create a Blank Theme

admin/create-theme/theme-block.php Outdated Show resolved Hide resolved
admin/create-theme/theme-media.php Show resolved Hide resolved
admin/create-theme/theme-templates.php Outdated Show resolved Hide resolved
Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This is a big PR! But I know it's because it's a combination of multiple branches. Here's my test results from following the test instructions. All these actions are from the options within the Site Editor.

Make changes to Global Styles and save the theme:

Changes reflected in the theme.json file ✅

Make changes to templates and save the theme:

  • Image assets included in the theme, referenced from the pattern, which is referenced from the template ❓

I noticed some characters in the image alt text are not correctly encoded: (Sorry, I think this is correct as it's coming through as HTML character codes within the alt text, which is what I'd expect.)

image

They're also not localized, but I think this could happen in a follow-up (I'm happy to open a separate issue).

When testing with a Cover block, I noticed that there are some encoding issues with the start and end of the PHP tag for referencing an image:

<!-- wp:cover {"url":"\u003c?php echo esc_url( get_stylesheet_directory_uri() ); ?\u003e/assets/images/flying-w.jpg","dimRatio":50,"customOverlayColor":"#88817d","isDark":false,"layout":{"type":"constrained"}} -->
  • Paragraph blocks localized in a pattern and referenced from a template ❌

I noticed when the paragraph is localized, some characters are not encoded correctly:

<!-- wp:paragraph --><p><?php echo __('The page youâ&#128;&#153;re looking doesnâ&#128;&#153;t seem to exist. Maybe try a search?', 'pixl');?></p>
<!-- /wp:paragraph -->
image

Otherwise, this looks like it's working well.

  • Templates with NO images or paragraphs not converted to a pattern ✅

Export a saved theme ✅

Clone a theme ✅

Create a Blank Theme ✅

I also tested based on the proposed changes from #162:

  • Remove ref attributes from the Navigation block ✅
  • Remove id attributes from Image and Cover blocks ✅
  • Remove wp-image-[id] classes from Image and Cover blocks ✅
  • Remove category IDs from query blocks ✅
  • Remove taxQuery attribute from query blocks ❌ taxQuery is not removed from the block

In summary, looks like there are some small issues with encoding and the taxQuery attribute but overall this is testing really well. Thank you for working on this, the code is looking much cleaner and better organised. Looking forward to this landing, especially the changes related to removing the site-specific attributes ✨

@madhusudhand
Copy link
Contributor

madhusudhand commented Mar 25, 2024

While testing, observed that patronizer copies content into new patterns instead of re-using existing patterns.
created a separate issue #533, as this isn't a newly introduced behavior.

Copy link
Contributor

@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.

Verified the changes for all the exported options and changes to templates. It works as expected. 🚢

Copy link
Member

@mikachan mikachan left a comment

Choose a reason for hiding this comment

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

This is working well for me too, I think this is good to bring in now 🎉

Copy link
Member

@vcanales vcanales left a comment

Choose a reason for hiding this comment

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

+1 to previous comments; this is testing well for me.

@pbking pbking merged commit 20bcc01 into trunk Mar 27, 2024
2 checks passed
@pbking pbking deleted the refactor/logic-out-of-api branch March 27, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants