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

Accurate sizes: Disable layout calculations for classic themes #1744

Merged
merged 3 commits into from
Dec 16, 2024

Conversation

mukeshpanchal27
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 commented Dec 16, 2024

Summary

This PR Checks the functionality of the image block in classic themes. After merging #1738, the sizes attribute for the classic theme has been producing incorrect values.

Issue Example:
When selecting the wide alignment for an image block, the sizes attribute is rendered as: sizes="(max-width: ) 100vw, ", This is incorrect. The expected output should be: sizes="(max-width: 1024px) 100vw, 1024px"

This PR aims to fix this issue and ensure the sizes attribute is generated correctly for classic themes.

Steps to reproduce the issue

  • Activate classic theme (TT1)
  • Add image block and select wide alignment
  • Check frontend.

@mukeshpanchal27 mukeshpanchal27 added [Type] Bug An existing feature is broken no milestone PRs that do not have a defined milestone for release [Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) labels Dec 16, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Dec 16, 2024
@mukeshpanchal27 mukeshpanchal27 removed the no milestone PRs that do not have a defined milestone for release label Dec 16, 2024
@mukeshpanchal27 mukeshpanchal27 added this to the auto-sizes 1.4.0 milestone Dec 16, 2024
@mukeshpanchal27 mukeshpanchal27 added no milestone PRs that do not have a defined milestone for release and removed no milestone PRs that do not have a defined milestone for release labels Dec 16, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review December 16, 2024 07:08
Copy link

github-actions bot commented Dec 16, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mukeshpanchal27 <[email protected]>
Co-authored-by: joemcgill <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@joemcgill
Copy link
Member

That makes sense because classic themes won't have a layout setting for this value. We could consider making a default guess, but disabling for now is probably sensable.

@joemcgill joemcgill merged commit 4c97d52 into trunk Dec 16, 2024
14 checks passed
@joemcgill joemcgill deleted the fix/check-classic-theme branch December 16, 2024 14:02
@felixarntz
Copy link
Member

@mukeshpanchal27 @joemcgill This PR looks good to me, though a related question: Do we already have an issue to explore adding a way to define those widths for classic themes?

@joemcgill joemcgill changed the title Accurate sizes: Check sizes issue for classic theme Accurate sizes: Disable layout calculations for classic themes Dec 16, 2024
@joemcgill
Copy link
Member

@felixarntz creating an API for defining layout values in classic themes is listed in the todo list for #760, but we've not yet created an issue for this yet. I wanted us to explore the implementation in the block theme context first before starting to define what that API should look like.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Enhanced Responsive Images Issues for the Enhanced Responsive Images plugin (formerly Auto Sizes) [Type] Bug An existing feature is broken
Projects
Status: Done 😃
Development

Successfully merging this pull request may close these issues.

3 participants