-
Notifications
You must be signed in to change notification settings - Fork 908
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
Show alert when avif file is used in social preview #21962
Show alert when avif file is used in social preview #21962
Conversation
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
Pull Request Test Coverage Report for Build 8f7622d3032382f4c7cf0b383c928dc67bc300b7Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
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.
CR 🏗️
Regarding the size warning we have. Should that not also be checked for the fallback, while you are at it?
Regarding your tech choices:
I implemented this check in the warppers (FacebookWrapper and TwitterWrapper) because it wouldn't make sense to have it in the more general ImageSelect component
Why does that not make sense?
refactoring this wouldn't make much sense also because the imageWarnings array is drilled down throughout the components' chain
Why would that stop you? You just need to adapt the source, i.e. the selector. Right?
Just thinking a bit on alternative solutions:
- You could make
getFacebookWarnings
and the like accept a second variable, the fallback URL. And then do this there. I'm not actually suggesting that is great, I'm just trying to get you to think a bit more inline with the current solutions. - Perhaps the warnings should always be calculated in a memoized selector. And taken out of the current set image with warning.
- Perhaps it is worth just adding a set warning action.
- Perhaps the
prepareFacebookPreviewImage
should encompass the fallback already so it can try to assess it too
Because the ImageSelect component doesn't need to (must not, I dare to say) know about application-specific logic: in our case we perform a check about the AVIF file format because the ImageSelect component is used inside our Social Appearance component, which is about Facebook and X, and both of them don't support the AVIF format.
I don't know... it seems a bit "dirty" to me because
I considered it but I usually adopt a "least-impactful" approach when dealing with ancient code (personal choice).
Because the warning array is drilled down where I need it already.
This is a suggestion I can agree upon 😁 In general, I felt that the fact the warnings array is available throughout the whole components hierarchy up to where I need it would make useless to add a selector. Moreover, we (currently) don't use it outside this hierarchy neither. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
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.
CR 🏗️
About the changelog entries for the social packages:
- The props are all lowercase. You have capitalized them in the changelog, why?
- The previews is just passing the image fallback. IF that deserves a changelog entry, maybe make it clear it is not adding a prop at all. It is passing the prop.
Co-authored-by: Igor <[email protected]>
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
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.
AT 🚧
Detected some problems:
- The AVIF in the content is not being picked up in Elementor as a fallback, so no warnings are displayed
- it does work when adding as social image
- might be unrelated to your PR so far 😓
- The copy of the warning is a bit strange. When selecting an AVIF image as Social image, the X image warning mentions it coming from the content. While we have no mechanism to determine where the fallback came from we now specify this in the message??
- When activating Premium the whole AVIF warnings don't work anymore. I'm presuming Premium needs to be build with this code from Free in order to fix that. But no test instructions or anything to indicate that
@pls78 Please be aware that following packages have been abandoned and are not actively maintained anymore:
Please consider using the other packages instead. |
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.
AT ✅
Context
Summary
This PR can be summarized in the following changelog entry:
ImageSelect
to determine which styling to apply.usingFallback
prop toSocialMetadataPreviewForm
component.imageFallbackUrl
prop toSocialPreviewEditor
component.Relevant technical choices:
FacebookWrapper
andTwitterWrapper
) because it wouldn't make sense to have it in the more generalImageSelect
component, as this limitation is specific to our implementation.manipulateuse theprops.imageWarnings
array instead of using an action because:selectMedia
prepareFacebookPreviewImage
andprepareTwitterPreviewImage
functions: refactoring this would require a non-negligible effort which is out of the scope of this PR. Moreover, theimageWarnings
array is drilled down throughout the components' chain where it is needed.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Social
tab in the Metabox and verify you see the following warning:X appearance
section, tooSelect image
and select any image of a supported typeRemove image
and check the warning is backSelect image
and select an image of AVIF type (you can select the same image you used in the post)Social media appearance
modal from the Yoast Sidebarwordpress-seo
in your Premium'svendor/yoast
foldervendor/yoast
create a symlink to thewordpress-seo
folder installation containing this branchRelevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #4496