-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Media & Text: don't use background-image #64981
Merged
Merged
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
03c82c2
media-text block: don't use background-image
sgomes d7b91d3
Add PR link to deprecated.js for future reference
sgomes 0c7a9ea
Fix PHP indentation
sgomes 9d10e74
Fix inconsistencies in some fixtures after markup changes
sgomes a374697
Update PHP tests to match new expected render
sgomes ac0ea5d
Add back old fixtures as new deprecated fixtures
sgomes f518c23
Improve deprecation comment wording in styles
sgomes f11a6b3
Remove 'not contains' checks from PHP tests
sgomes 311f447
Rename new CSS class to is-image-fill-element
sgomes File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next
Next commit
media-text block: don't use background-image
- Loading branch information
commit 03c82c28ab5a3abedd25dea652befbd408087d72
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
export function imageFillStyles( url, focalPoint ) { | ||
return url | ||
? { | ||
objectPosition: focalPoint | ||
? `${ Math.round( focalPoint.x * 100 ) }% ${ Math.round( | ||
focalPoint.y * 100 | ||
) }%` | ||
: `50% 50%`, | ||
} | ||
: {}; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can avoid the new class name in order to keep back compat for older versions. @Mamaduka do you see another way around for this? 🤔
If we can't avoid it let's find a better name without the version number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps something that references the new mechanism we're using for these? Here are some ideas:
is-image-fill-no-bg
is-image-fill-object
is-image-fill-element
Alternatively, we could take the opportunity to align more with the wording in the UI ("Crop image to fill"), so:
is-image-crop-to-fill
is-image-cropped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for why I opted for a new classname in the first place, let me explain in detail.
The issue is that we need new styles because of the markup changes, and these styles are incompatible with the previous ones because of the nature of these changes (changing display behaviour for the
<img>
element).We want posts with the old markup to keep working with new releases, without requiring any user intervention, because it's unacceptable to break them.
This leaves us with three options, as I understand:
Replace the classname
The idea here is to create a new classname for the "Crop image to fill" option, to replace the previous one. We keep the old styles under the old name as well, to ensure backward compatibility.
Markup example:
This is the option I went with in this PR.
Add another classname
The idea here is to keep the old classname for the "Crop image to fill" option, but add a new one to indicate the new markup is being used. We keep the old styles as-is under the old name, to ensure backward compatibility; the new styles override them.
Markup example:
I opted against this approach, since it would have meant additional CSS to return the properties changed in
is-image-fill
to their default values, and it didn't seem to come with significant benefits that I could think of at the time.There is one that didn't occur to me, however. Adding a new class would mean that we wouldn't need to remove the old classname when rendering the block dynamically, which would mean slightly less confusing PHP code.
Upgrade the markup
Another option would be to treat the block as dynamic, and rewrite its content in PHP if the "Crop image to fill" option is enabled. This would ensure the new markup is always used, removing the need to preserve backwards compatibility with styles. This means we could drop the old styles, and simply have the same classname with the new styles.
Currently, this block is only dynamic when using a featured image. I opted against this approach because it would have meant further usage of dynamic blocks, which can result in increased server processing times.
Other approaches?
Are there any other approaches I'm not aware of that we should be considering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless we get feedback for a possible different approach, the new class name seems fine to me. Maybe the name could be something like
is-image-fill-element
you suggested, but this is the easiest change we can do later, before merging.