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

Add compat support for twentytwenty core theme #3403

Merged
merged 42 commits into from
Oct 18, 2019

Conversation

schlessera
Copy link
Collaborator

@schlessera schlessera commented Oct 1, 2019

Adds support for the new twentytwenty core theme that will be part of WordPress 5.3.

Changes and todo:

Fixes #3342.

@schlessera schlessera added Enhancement New feature or improvement of an existing one Sanitizers Integration: WP Core labels Oct 1, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Oct 1, 2019
@westonruter westonruter added this to the v1.3.1 milestone Oct 1, 2019
@schlessera schlessera force-pushed the add/3342-twentytwenty-support branch from b8b4d97 to 776b952 Compare October 7, 2019 10:08
@schlessera schlessera force-pushed the add/3342-twentytwenty-support branch 2 times, most recently from 1f86538 to 5185ea4 Compare October 8, 2019 16:45
@schlessera
Copy link
Collaborator Author

I've finished building generic replacements for the "toggles" and "modals" that the theme provides. This now makes it functional and usable.

There are still lots of fine-tuning issues left, but the bulk of the work seems to be done.

@schlessera schlessera force-pushed the add/3342-twentytwenty-support branch from 7d53aab to 2738a5e Compare October 10, 2019 06:49
@schlessera
Copy link
Collaborator Author

The "spacing discrepancies" mentioned above seem to be related to the <amp-image> element or its parent <figure> element, and the way they size themselves. However, I couldn't find out the specific problem yet.

Image 2019-10-12 at 9 31 18 AM

The <amp-img> contains an image at 580x300px and is also explicitly set to these dimensions. The computed height is 300px.

For the surrounding <figure>, though, the computed height is 308px. It shows a small gap at the bottom of the image where it leaves 8px of space. I couldn't find any CSS that would account for this gap.

I assume for now that this spacing is incorrectly calculated inside of <amp-img>'s JS code.

@schlessera
Copy link
Collaborator Author

It looks like the "broken <pre> tag" issue above points to a bigger problem of parsing/escaping HTML.

The issue is caused by the cat picture in the <pre> tag (not the opening and closing brackets around the neck):
Image 2019-10-12 at 11 42 05 AM
This results in the following broken HTML:
Image 2019-10-12 at 11 42 12 AM

So part of the cat is output as actual HTML tags to the browser, breaking the markup.

@swissspidy swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019
@schlessera
Copy link
Collaborator Author

@westonruter I couldn't find any tests for theme compat for previous themes? Do you want me to still invest the time to write tests for the remaining items (see #3403 (comment))?

It will be quite a bit of work to build such a theme test from scratch, and it will probably not test much more then my current assumptions anyway, as I don't have visual regression testing at my disposal yet. So it might not be worth the effort for now, and would be better to get this into QA as quickly as possible?

includes/sanitizers/class-amp-core-theme-sanitizer.php Outdated Show resolved Hide resolved
includes/utils/class-amp-dom-utils.php Outdated Show resolved Hide resolved
includes/utils/class-amp-dom-utils.php Outdated Show resolved Hide resolved
includes/utils/class-amp-dom-utils.php Outdated Show resolved Hide resolved
@westonruter westonruter changed the title [WIP] Add compat support for twentytwenty core theme Add compat support for twentytwenty core theme Oct 18, 2019
…twentytwenty-support

* 'develop' of github.com:ampproject/amp-wp: (73 commits)
  Mock HTTP request for Test_AMP_WordPress_TV_Embed_Handler
  Add test case
  Fix issues with image display (#3555)
  Fix infinite loop when uploading videos (#3553)
  Fix styling on remove button. (#3554)
  Test for XML processing instruction
  Undo over-eager rename of auto_accept_sanitization
  Rename auto-accept to accept-by-default
  Rename filter to amp_validation_error_auto_sanitized
  Fix jumping blocks when changing fonts (#3529)
  Update dependency browserslist to v4.7.1 (#3551)
  Make sure that isPageBlock always returns boolean.
  Fix and improve e2e tests.
  Remove unnessary check for block. Use shift over access first element of array.
  Use isPageBlock for cleaner code.
  Fix minor grammar issues
  Add docblock for `amp_is_sanitization_auto_accepted` filter
  Align arrows
  Update tests to use filter to set sanitization auto acceptance
  Make AMP_Validation_Manage::is_sanitization_auto_accepted() filterable
  ...
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Amazing work. What an epic effort. 🤝

@westonruter westonruter merged commit 197a600 into develop Oct 18, 2019
@westonruter westonruter deleted the add/3342-twentytwenty-support branch October 18, 2019 21:54
@@ -1023,6 +1023,18 @@ static function() use ( $args ) {
right: calc(100% + 2rem);
}

.admin-bar .menu-modal {
/* Use padding to shift down modal because amp-lightbox has top:0 !important. */
padding-top: 32px;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, I just noticed I stored this in my browser, but not in the PR...
Have to watch out for this next time, I thought it was fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, how did you store it in your browser?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA Enhancement New feature or improvement of an existing one Integration: WP Core Sanitizers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add theme support for Twenty Twenty (and WordPress 5.3)
4 participants