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.

Sign up for GitHub

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

Jump to bottom

Update legacy theme styles to add responsiveness to featured image #7560

Merged
merged 16 commits into from
Jul 7, 2023
Merged

Update legacy theme styles to add responsiveness to featured image #7560

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
558578b
Update legacy theme styles to add responsiveness to featured images
thelovekesh Jun 6, 2023
662e5b8
Merge branch 'develop' into fix/legacy-theme-img-responsiveness
thelovekesh Jun 9, 2023
2b2b9c3
Update legacy theme featured image style
thelovekesh Jul 6, 2023
7f752ab
Merge branch 'develop' into fix/legacy-theme-img-responsiveness
thelovekesh Jul 6, 2023
d62b5c2
Fix maridb container init in GH workflows
thelovekesh Jul 6, 2023
2b21baa
Decouple composer normalize from phpcs job
thelovekesh Jul 6, 2023
843fd55
Fix phpstan errors
thelovekesh Jul 6, 2023
5b2cf50
Remove fetchpriority from test markup
thelovekesh Jul 6, 2023
8f61904
Update deprecated ` WP_Scripts::print_inline_script` with `WP_Scripts…
thelovekesh Jul 6, 2023
14e58fb
Update `PolyfillsTest::get_inline_script()`
thelovekesh Jul 6, 2023
d00a239
Remove clamp values from tests as they can be dynamic after new WP re…
thelovekesh Jul 7, 2023
403af64
Add WP_DEVELOPMENT_MODE to theme in phpunit bootstrap file
thelovekesh Jul 7, 2023
5247813
Remove `$display` param from `::get_inline_script()` function
thelovekesh Jul 7, 2023
e65ef36
Revert "Add WP_DEVELOPMENT_MODE to theme in phpunit bootstrap file"
thelovekesh Jul 7, 2023
be8c3fb
Remove dependency from `wp_theme_has_theme_json()` to check if theme …
thelovekesh Jul 7, 2023
892fa93
Update theme_has_theme_json to match core-patched version
westonruter Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 40 additions & 17 deletions .github/workflows/build-test-measure.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,36 @@ jobs:
repo-token: '${{ secrets.GITHUB_TOKEN }}'
report-json: 'lint-js-report.json'

#-----------------------------------------------------------------------------------------------------------------------

normalize-composer:
name: 'Normalize composer.json'
needs: pre-run
if: needs.pre-run.outputs.changed-php-count > 0 || needs.pre-run.outputs.changed-gha-workflow-count > 0
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v3

- name: Setup PHP
uses: shivammathur/setup-php@v2
with:
php-version: '8.1'
coverage: none

- name: Get composer-normalize.phar
run: |
wget https://github.com/ergebnis/composer-normalize/releases/latest/download/composer-normalize.phar
chmod +x composer-normalize.phar

- name: Validate composer.json
run: composer --no-interaction validate --no-check-all

- name: Normalize composer.json
run: |
composer config --no-interaction --no-plugins allow-plugins.ergebnis/composer-normalize true
./composer-normalize.phar --dry-run

#-----------------------------------------------------------------------------------------------------------------------

lint-php:
Expand Down Expand Up @@ -189,18 +219,9 @@ jobs:
- name: Install Composer dependencies
run: composer install --prefer-dist --optimize-autoloader --no-progress --no-interaction

- name: Validate composer.json
run: composer --no-interaction validate --no-check-all

- name: Detect coding standard violations (PHPCS)
run: vendor/bin/phpcs -q --report=checkstyle --runtime-set ignore_errors_on_exit 1 --runtime-set ignore_warnings_on_exit 1 | cs2pr --graceful-warnings

- name: Normalize composer.json
run: |
composer config --no-interaction --no-plugins allow-plugins.ergebnis/composer-normalize true
composer require --no-interaction --dev ergebnis/composer-normalize --ignore-platform-reqs
composer --no-interaction normalize --dry-run

#-----------------------------------------------------------------------------------------------------------------------

static-analysis-php:
Expand Down Expand Up @@ -362,12 +383,13 @@ jobs:
mysql:
image: mariadb:latest
env:
MYSQL_ALLOW_EMPTY_PASSWORD: true
MYSQL_ROOT_PASSWORD:
MYSQL_DATABASE: wordpress_test
MARIADB_ALLOW_EMPTY_ROOT_PASSWORD: true
MARIADB_DATABASE: wordpress_test
MARIADB_MYSQL_LOCALHOST_USER: 1
MARIADB_MYSQL_LOCALHOST_GRANTS: USAGE
ports:
- 3306
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
options: --health-cmd="healthcheck.sh --su-mysql --connect --innodb_initialized" --health-interval=10s --health-timeout=5s --health-retries=3
continue-on-error: ${{ matrix.experimental == true }}
strategy:
fail-fast: false
Expand Down Expand Up @@ -606,12 +628,13 @@ jobs:
mysql:
image: mariadb:latest
env:
MYSQL_ALLOW_EMPTY_PASSWORD: true
MYSQL_ROOT_PASSWORD:
MYSQL_DATABASE: wordpress_test
MARIADB_ALLOW_EMPTY_ROOT_PASSWORD: true
MARIADB_DATABASE: wordpress_test
MARIADB_MYSQL_LOCALHOST_USER: 1
MARIADB_MYSQL_LOCALHOST_GRANTS: USAGE
ports:
- 3306
options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=3
options: --health-cmd="healthcheck.sh --su-mysql --connect --innodb_initialized" --health-interval=10s --health-timeout=5s --health-retries=3
continue-on-error: ${{ matrix.experimental == true }}
strategy:
fail-fast: false
Expand Down
3 changes: 2 additions & 1 deletion bin/ci/install-wp-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ install_db() {
fi

# create database
mysqladmin create "$DB_NAME" --user="$DB_USER" --password="$DB_PASS""$EXTRA"
mariadb-admin create $DB_NAME --user="$DB_USER" --password="$DB_PASS"$EXTRA || \
mysqladmin create "$DB_NAME" --user="$DB_USER" --password="$DB_PASS"$EXTRA
}

install_wp
Expand Down
2 changes: 2 additions & 0 deletions includes/embeds/class-amp-core-block-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,7 @@ private function process_text_widgets( Document $dom ) {
foreach ( $dom->xpath->query( '//div[ @class = "textwidget" ]' ) as $text_widget ) {
// Restore the width/height attributes which were preserved in preserve_widget_text_element_dimensions.
foreach ( $dom->xpath->query( sprintf( './/*[ @%s or @%s ]', self::AMP_PRESERVED_WIDTH_ATTRIBUTE_NAME, self::AMP_PRESERVED_HEIGHT_ATTRIBUTE_NAME ), $text_widget ) as $element ) {
/** @var DOMElement $element */
if ( $element->hasAttribute( self::AMP_PRESERVED_WIDTH_ATTRIBUTE_NAME ) ) {
$element->setAttribute( Attribute::WIDTH, $element->getAttribute( self::AMP_PRESERVED_WIDTH_ATTRIBUTE_NAME ) );
$element->removeAttribute( self::AMP_PRESERVED_WIDTH_ATTRIBUTE_NAME );
Expand All @@ -586,6 +587,7 @@ private function process_text_widgets( Document $dom ) {
* responsive so this is built-in. Note also the style rule for .wp-video in amp-default.css.
*/
foreach ( $dom->xpath->query( './/div[ @class = "wp-video" and @style ]', $text_widget ) as $element ) {
/** @var DOMElement $element */
$element->removeAttribute( 'style' );
}
}
Expand Down
2 changes: 2 additions & 0 deletions includes/embeds/class-amp-tumblr-embed-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ public function sanitize_raw_embeds( Document $dom ) {
);
$overflow_element->textContent = __( 'See more', 'amp' );
$amp_element->appendChild( $overflow_element );

/** @var DOMElement $placeholder */
$placeholder->setAttribute( Attribute::PLACEHOLDER, '' );
$amp_element->appendChild( $placeholder );

Expand Down
1 change: 1 addition & 0 deletions includes/sanitizers/class-amp-style-sanitizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,7 @@ public function sanitize() {
}
} else {
foreach ( $styled_elements as $element ) {
/** @var DOMElement $element */
$attr = $element->getAttributeNode( Attribute::STYLE );
if ( $attr && preg_match( '/!\s*important/i', $attr->value ) ) {
ValidationExemption::mark_node_as_px_verified( $attr );
Expand Down
5 changes: 5 additions & 0 deletions templates/style.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,11 @@
.amp-wp-article-featured-image {
margin: 0 0 1em;
}
.amp-wp-article-featured-image img:not(amp-img) {
max-width: 100%;
height: auto;
margin: 0 auto;
}
.amp-wp-article-featured-image amp-img {
margin: 0 auto;
}
Expand Down
1 change: 1 addition & 0 deletions tests/php/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use Yoast\WPTestUtils\WPIntegration;

define( 'WP_DEVELOPMENT_MODE', 'theme' );
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be plugin?

Suggested change
define( 'WP_DEVELOPMENT_MODE', 'theme' );
define( 'WP_DEVELOPMENT_MODE', 'plugin' );

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it should be. I am just testing a few scenarios with theme.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@westonruter wp_theme_has_theme_json() now depends on wp_get_development_mode() which is theme and WP_RUN_CORE_TESTS constant but setting up WP_RUN_CORE_TESTS is breaking our multiple test cases so I think for now we have to keep WP_DEVELOPMENT_MODE as theme.

Copy link
Member

Choose a reason for hiding this comment

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

Is the problem that wp_theme_has_theme_json() is sending back stale results? Does this reflect a problem with caching?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. In these tests, we are changing the theme to TT3 to test reader theme features from theme.json. After changing the theme wp_theme_has_theme_json() returns false as the previous theme was not a theme with theme.json which is a cached result AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

I think this then reflects a bug in wp_theme_has_theme_json. In particular, it is not varying the cache by the theme. There needs to be another static variable in wp_theme_has_theme_json which captures the theme that was checked, or else to let $theme_has_support be a mapping of theme slug to boolean.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah makes sense. It should have some logic to revalidate the cache in case the theme is being changed.

Copy link
Member

Choose a reason for hiding this comment

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

Core patch created: WordPress/wordpress-develop#4816

define( 'TESTS_PLUGIN_DIR', dirname( dirname( __DIR__ ) ) );

// When run in wp-env context, set the test config file path.
Expand Down
29 changes: 26 additions & 3 deletions tests/php/src/Admin/PolyfillsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,11 +85,11 @@ public function test_registration() {

// These should pass in WP < 5.6.
$this->assertTrue( wp_script_is( 'lodash', 'registered' ) );
$this->assertStringContainsString( '_.noConflict();', wp_scripts()->print_inline_script( 'lodash', 'after', false ) );
$this->assertStringContainsString( '_.noConflict();', self::get_inline_script( 'lodash', 'after' ) );

$this->assertTrue( wp_script_is( 'wp-api-fetch', 'registered' ) );
$this->assertStringContainsString( 'createRootURLMiddleware', wp_scripts()->print_inline_script( 'wp-api-fetch', 'after', false ) );
$this->assertStringContainsString( 'createNonceMiddleware', wp_scripts()->print_inline_script( 'wp-api-fetch', 'after', false ) );
$this->assertStringContainsString( 'createRootURLMiddleware', self::get_inline_script( 'wp-api-fetch', 'after' ) );
$this->assertStringContainsString( 'createNonceMiddleware', self::get_inline_script( 'wp-api-fetch', 'after' ) );

$this->assertTrue( wp_script_is( 'wp-hooks', 'registered' ) );
$this->assertTrue( wp_script_is( 'wp-i18n', 'registered' ) );
Expand All @@ -99,4 +99,27 @@ public function test_registration() {

$this->assertTrue( wp_style_is( 'wp-components', 'registered' ) );
}

/**
* Get inline script.
*
* @param string $handle Script handle.
* @param string $position Script position.
* @param bool $display Whether to display the script.
*
* @return string
*/
public static function get_inline_script( $handle, $position = 'after', $display = false ) {
if ( method_exists( wp_scripts(), 'get_inline_script_tag' ) ) {
$script = wp_scripts()->get_inline_script_tag( $handle, $position );

if ( ! $script ) {
return false;
}

return $script;
} else {
return wp_scripts()->print_inline_script( $handle, $position, $display );
thelovekesh marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
13 changes: 7 additions & 6 deletions tests/php/src/Editor/EditorSupportTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use AmpProject\AmpWP\Infrastructure\Service;
use AmpProject\AmpWP\Tests\Helpers\WithBlockEditorSupport;
use AmpProject\AmpWP\Tests\TestCase;
use AmpProject\AmpWP\Tests\Admin\PolyfillsTest;

/** @coversDefaultClass \AmpProject\AmpWP\Editor\EditorSupport */
final class EditorSupportTest extends TestCase {
Expand Down Expand Up @@ -71,15 +72,15 @@ public function test_supports_current_screen( $post_type_uses_block_editor, $pos
/** @covers ::maybe_show_notice() */
public function test_dont_show_notice_if_no_screen_defined() {
$this->instance->maybe_show_notice();
$this->assertFalse( wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false ) );
$this->assertFalse( PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' ) );
}

/** @covers ::maybe_show_notice() */
public function test_dont_show_notice_for_unsupported_post_type() {
$this->setup_environment( true, false );

$this->instance->maybe_show_notice();
$this->assertFalse( wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false ) );
$this->assertFalse( PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' ) );
}

/** @covers ::maybe_show_notice() */
Expand All @@ -92,11 +93,11 @@ public function test_show_notice_for_supported_post_type() {

$this->instance->maybe_show_notice();
if ( $this->instance->is_current_screen_block_editor_for_amp_enabled_post_type() ) {
$this->assertFalse( wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false ) );
$this->assertFalse( PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' ) );
} else {
$this->assertStringContainsString(
'AMP functionality is not available',
wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false )
PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' )
);
}
}
Expand All @@ -108,7 +109,7 @@ public function test_maybe_show_notice_for_unsupported_user() {

$this->instance->maybe_show_notice();

$this->assertFalse( wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false ) );
$this->assertFalse( PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' ) );
}

/** @covers ::maybe_show_notice() */
Expand All @@ -125,7 +126,7 @@ public function test_maybe_show_notice_for_gutenberg_4_9() {
wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) );

$this->instance->maybe_show_notice();
$inline_script = wp_scripts()->print_inline_script( 'wp-edit-post', 'after', false );
$inline_script = PolyfillsTest::get_inline_script( 'wp-edit-post', 'after' );
$this->assertStringContainsString( 'AMP functionality is not available', $inline_script );
}
}
7 changes: 3 additions & 4 deletions tests/php/src/ReaderThemeSupportFeaturesTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,16 +483,15 @@ public function test_print_theme_support_styles_reader( $is_legacy ) {
$this->assertStringContainsString( '<style id="amp-wp-theme-support-editor-font-sizes">', $output );
$this->assertStringContainsString( 'font-size: clamp(', $output );
$this->assertStringContainsString( '+ ((', $output );
$this->assertStringContainsString( ':root .has-small-font-size { font-size: clamp(0.875rem, 0.875rem + ((1vw - 0.48rem) * 0.24), 1rem); }', $output );
$this->assertStringContainsString( ':root .has-small-font-size { font-size: clamp(0.875rem', $output );
}

// Assert spacing size custom properties.
if ( $this->call_private_method( $this->instance, 'theme_has_theme_json' ) && function_exists( 'wp_get_global_settings' ) ) {
$this->assertStringContainsString( '<style id="amp-wp-theme-support-spacing-sizes-custom-properties">', $output );
$this->assertStringContainsString( ':root {', $output );
$this->assertStringContainsString( '--wp--preset--spacing--30: clamp(1.5rem, 5vw, 2rem);', $output );
$this->assertStringContainsString( 'clamp(1.8rem, 1.8rem + ((1vw - 0.48rem) * 2.885), 3rem);', $output );
$this->assertStringContainsString( '--wp--preset--spacing--80: clamp(7rem, 14vw, 11rem);', $output );
$this->assertStringContainsString( '--wp--preset--spacing--30:', $output );
$this->assertStringContainsString( '--wp--preset--spacing--80:', $output );
$this->assertStringContainsString( '}', $output );
$this->assertStringContainsString( '</style>', $output );
}
Expand Down
3 changes: 3 additions & 0 deletions tests/php/test-amp-gallery-embed-handler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,9 @@ public function test__conversion( $source, $expected, $use_legacy_mode = false )
// Remove lazy loading attribute.
$content = preg_replace( '/\s+loading="lazy"/', '', $content );

// Remove fetchpriority attribute.
$content = preg_replace( '/\s+fetchpriority="high"/', '', $content );

$this->assertEquals(
$this->normalize( $expected ),
$this->normalize( $content )
Expand Down
Loading