-
Notifications
You must be signed in to change notification settings - Fork 1
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
wip: fixing all image checks and ListingImage #861
base: main
Are you sure you want to change the base?
Conversation
src/components/ItaliaTheme/Blocks/Listing/CardWithImage/CardWithImageDefault.jsx
Outdated
Show resolved
Hide resolved
src/helpers/images.js
Outdated
const contentHasImage = (item, customValidation = true) => { | ||
return (item?.image_field || item?.image_scales) && customValidation; | ||
}; |
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.
Non sono sicuro del valore aggiunto che ci dà il parametro customValidation
, se l'intero check viene passato dal chiamante, praticamente fare
const showImage = contentHasImage(item, customValidation)
è equivalente a fare
const showImage = contentHasImage(item) && customValidation
E se l'intenzione è questa forse conviene non metterlo direttamente il parametro e lasciare il metodo il più lineare possibile.
the default fields in a JSON response for a content in plone.restapi | ||
The additional argument, customValidation, can be provided for custom checks | ||
@param {Object} item - The Plone item/brain/content |
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.
Fatto così mi sembra che funzioni solo con brain/summary e non con oggetti completi derivanti da una GET su un contesto o da una ricerca fullobjects
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.
Si, hai ragione, avere un fullobject succede solo in un paio di card che hanno showimage a false (e quindi non lo vedrai mai), ma succede. Porto qui il check di image su volto, ma senza calcolare le scales
const contentHasImage = (item) => {
if(!item) return false
const isFromRealObject = !item?.image_scales;
const imageFieldWithDefault = item?.image_field || 'image';
const image = isFromRealObject
? item[imageFieldWithDefault]
: item.image_scales[imageFieldWithDefault]?.[0];
return Boolean(image);
};
Dovrebbe essere ok per tutte le casistiche
<Image | ||
item={item} | ||
imageField={cooked_image_field} | ||
alt={item.title ?? ''} |
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.
Alt vuoto come era prima va benissimo in realtà perché il titolo è poco dopo, poi in generale non cambierei questo comportamento in questa PR a meno di segnalazioni particolari, perché è out of scope
Fix di tutti i check image truthish, in quanto image e' un istanza di un React Element e non puo' essere mai falsish, non rappresenta il possibile ritorno del componente mai.
Da finire di testare per bene, ho iniziato qui https://v3.io-comune.redturtle.it/test-martina/test-fix-img-null-check-bug
PS: Alcune occorrenze simili esistono anche in Volto tipo PreviewImage, se li usiamo e' da scoprire, non ci sono arrivata ancora