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 legacy theme styles to add responsiveness to featured image #7560

Merged
merged 16 commits into from
Jul 7, 2023

Conversation

thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jun 6, 2023

Summary

Fixes #7550

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023

Plugin builds for 892fa93 are ready 🛎️!

@westonruter westonruter added this to the v2.4.2 milestone Jun 28, 2023
templates/style.php Outdated Show resolved Hide resolved
@thelovekesh thelovekesh force-pushed the fix/legacy-theme-img-responsiveness branch from f912cba to d62b5c2 Compare July 6, 2023 15:43
@thelovekesh thelovekesh force-pushed the fix/legacy-theme-img-responsiveness branch 2 times, most recently from 2321c08 to 14e58fb Compare July 7, 2023 16:11
@@ -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

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.

Great work!

@westonruter westonruter merged commit 3396a0c into develop Jul 7, 2023
34 checks passed
@westonruter westonruter deleted the fix/legacy-theme-img-responsiveness branch July 7, 2023 19:47
@thelovekesh thelovekesh added Bug Something isn't working Enhancement New feature or improvement of an existing one P2 Low priority WS:Core Work stream for Plugin core and removed Bug Something isn't working Enhancement New feature or improvement of an existing one P2 Low priority WS:Core Work stream for Plugin core labels Jul 13, 2023
@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Reader Mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature image stretched on Legacy theme, when native image is being used.
2 participants