Show alert when avif file is used in social preview #21962
Conversation
pls78
added
the
changelog: enhancement
|
@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 67da9d8df481248df4b69940171cc88da52a3ae7Details
💛 - 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.
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
getFacebookWarningsand 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
prepareFacebookPreviewImageshould encompass the fallback already so it can try to assess it too
| @@ -72,7 +68,7 @@ function ImageSelect( props ) { | |||
| </button> | |||
| } | |||
| { | |||
| showWarnings && <div role="alert"> | |||
| showWarnings && <div role="alert" className="yst-mt-4"> | |||
There was a problem hiding this comment.
@yoast/components uses styled-components. Please don't combine that with Tailwind, for which no configuration exists within this package.
There was a problem hiding this comment.
Regarding your changelog entry for this file
[components/image-select 0.0.1] Makes possible to show warnings related to the fallback image file format.
- It should state the package name, e.g.
@yoast/components. You can find this in the closest package.json file - Adding required props is a breaking API change for components. So that would mean a major release. Not a patch, e.g.
1.0.0indicator. - If you read it again. It does not really sound like proper English, does it? "Makes possible"
| @@ -91,6 +87,7 @@ export default ImageSelect; | |||
| ImageSelect.propTypes = { | |||
| defaultImageUrl: PropTypes.string, | |||
| imageUrl: PropTypes.string, | |||
| imageFallbackUrl: PropTypes.string.isRequired, | |||
There was a problem hiding this comment.
Why is this required? Even the default URL is not.
| @@ -15,12 +15,8 @@ import Alert from "../Alert"; | |||
| function ImageSelect( props ) { | |||
| const imageSelected = props.usingFallback === false && props.imageUrl !== ""; | |||
| const previewImageUrl = props.imageUrl || props.defaultImageUrl || ""; | |||
| const showWarnings = props.warnings.length > 0 && imageSelected; | |||
|
|
|||
| let imageClassName = showWarnings ? "yoast-image-select__preview yoast-image-select__preview-has-warnings" : "yoast-image-select__preview"; | |||
There was a problem hiding this comment.
If removing this class from ever existing. Why not mention it in the changelog, and why keep the style itself in the CSS file?
Funnily enough, this seems like the margin you went to solve with a Tailwind class later on?
So are you sure you can just remove this?
| if ( previewImageUrl === "" ) { | ||
| imageClassName = "yoast-image-select__preview yoast-image-select__preview--no-preview"; | ||
| } | ||
| const showWarnings = props.warnings.length > 0 && ( imageSelected || props.imageFallbackUrl !== "" ); |
There was a problem hiding this comment.
Why does this component need to keep track of this when in fact it does nothing with the imageFallbackUrl besides this?
Should you not change the usingFallback and/or imageUrl values instead?
Is this all to not even show the image? If so, that seems counter to what this always has done. And instead of untangling that (don't check imageSelected but let the callees determine that), more tangling is done with this extra prop.
| export const useFallbackWarning = ( imageFallbackUrl, imageUrl, imageWarnings ) => { | ||
| useEffect( () => { | ||
| if ( imageUrl === "" ) { | ||
| imageWarnings.length = 0; |
There was a problem hiding this comment.
It seems highly inappropriate to override an array calculated value. Do you mean to say you would want to imageWarnings = [] instead?
Even then, it is generally nicer to not mess with the input values, but instead copy and return your own variant instead. For more predictable code. It is easy to overlook your current and this is how bugs are born.
| useEffect( () => { | ||
| if ( imageUrl === "" ) { | ||
| imageWarnings.length = 0; | ||
| if ( imageFallbackUrl.toLowerCase().endsWith( ".avif" ) ) { |
There was a problem hiding this comment.
Why just test for .avif? Would it not be nicer to test against the allowed instead? E.g. the list below on line 24.
Also. How many URLs actually include the file extension? This seems a bit flimsy. But I suppose there is no way around that unless we want to go full on image processing.
And probably more the reason why it works like it currently does. Only when we know more, i.e. when setting the image, do we have a chance of more metadata.
| imageWarnings.push( warningMessage ); | ||
| } | ||
| } | ||
| }, [ imageFallbackUrl, imageUrl ] ); |
There was a problem hiding this comment.
To me, a useEffect hook where the rest of the code lives in containers format is very strange.
There is no need to keep watching it when we already control when it changes.
I think this highlights the fact that the selectors are not doing a proper job for you.
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
FacebookWrapperandTwitterWrapper) because it wouldn't make sense to have it in the more generalImageSelectcomponentprops.imageWarningsarray instead of using an action because:selectMediaprepareFacebookPreviewImageandprepareTwitterPreviewImagefunctions: refactoring this wouldn't make much sense also because theimageWarningsarray is drilled down throughout the components' chainuseFallbackWarningcustom hook is called ( this is done to prevent duplicating the warning when the user accesses theSocial media appearancemodal)Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Socialtab in the Metabox and verify you see the following warning:X appearancesection, tooSelect imageand select any image of a supported typeRemove imageand check the warning is backSelect imageand select an image of AVIF type (you can select the same image you used in the post)Social media appearancemodal from the Yoast SidebarRelevant 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 theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes #4496