Skip to content

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

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
Merged
westonruter merged 16 commits into from
Jul 7, 2023
Merged

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

westonruter merged 16 commits into from
Jul 7, 2023

Conversation

@thelovekesh
Copy link
Collaborator

@thelovekesh thelovekesh commented Jun 6, 2023 •
edited
Loading

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

@thelovekesh thelovekesh requested a review from westonruter June 6, 2023 15:13
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2023 •
edited
Loading

Plugin builds for 892fa93 are ready 🛎️!

@westonruter westonruter added this to the v2.4.2 milestone Jun 28, 2023
westonruter
westonruter reviewed Jun 28, 2023
View reviewed changes
@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
westonruter
westonruter reviewed Jul 7, 2023
View reviewed changes
tests/php/bootstrap.php Outdated

use Yoast\WPTestUtils\WPIntegration;

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

@westonruter westonruter Jul 7, 2023

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

@thelovekesh thelovekesh Jul 7, 2023

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

@thelovekesh thelovekesh Jul 7, 2023

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

@westonruter westonruter Jul 7, 2023

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

@thelovekesh thelovekesh Jul 7, 2023

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

@westonruter westonruter Jul 7, 2023

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

@thelovekesh thelovekesh Jul 7, 2023

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

@westonruter westonruter Jul 7, 2023

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

westonruter
westonruter reviewed Jul 7, 2023
View reviewed changes
@thelovekesh thelovekesh force-pushed the fix/legacy-theme-img-responsiveness branch from c7ef24f to be8c3fb Compare July 7, 2023 17:29
@thelovekesh thelovekesh requested a review from westonruter July 7, 2023 17:33
@thelovekesh thelovekesh mentioned this pull request Jul 7, 2023
Draft
2 tasks
@westonruter westonruter mentioned this pull request Jul 7, 2023
Closed
westonruter
westonruter approved these changes Jul 7, 2023
View reviewed changes
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 enabled auto-merge July 7, 2023 19:41
@westonruter westonruter merged commit 3396a0c into develop Jul 7, 2023
@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

Reviewers

@westonruter westonruter westonruter approved these changes

Assignees

No one assigned

Labels

Changelogged Whether the issue/PR has been added to release notes. Reader Mode

Projects

None yet

Milestone

v2.4.2

Development

Successfully merging this pull request may close these issues.

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

3 participants

@thelovekesh @westonruter