-
Notifications
You must be signed in to change notification settings - Fork 7
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
Make image ratio and sizes configurable #999
Conversation
…ded image formats code
@@ -50,9 +52,17 @@ export const isExternal = url => | |||
* @returns {string} | |||
*/ | |||
export const getActualImageSource = (src, { width, height }) => { | |||
if (src && src.includes('images.shopgate.services/v2/images')) { | |||
const { fillColor = 'FFFFFF' } = getThemeSettings('AppImages') || {}; | |||
const format = isAndroidOs ? 'webp' : 'jpeg'; |
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.
Extensions also checked if sufix is '.png' to ask for png format
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.
not sure if this is needed. @fkloes what do you think?
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.
Sorry, but i'm not sure, if i get the question? Can you please elaborate it a bit?
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.
When original image is PNG , the image-optimizer
extension checks the suffix to return png
format
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.
@fkloes the image optimize ext was doing this check here https://stash.localdev.cc/projects/SGX/repos/image-optimizer/browse/extension/helpers/generatePlatformImageUrl.js#37
But i don't know if this is really used and if it should be also included to the frontend.
...heme-gmd/pages/Favorites/components/FavoritesList/components/Item/components/Image/index.jsx
Outdated
Show resolved
Hide resolved
themes/theme-gmd/pages/Product/components/Media/components/ProductImageSlider/index.jsx
Outdated
Show resolved
Hide resolved
...me-ios11/pages/Favorites/components/FavoritesList/components/Item/components/Image/index.jsx
Outdated
Show resolved
Hide resolved
themes/theme-ios11/pages/Product/components/Media/components/ProductImageSlider/index.jsx
Outdated
Show resolved
Hide resolved
themes/theme-gmd/pages/Product/components/Media/components/ProductImageSlider/index.jsx
Outdated
Show resolved
Hide resolved
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 did my first review of the code, but i wasn't able to do functional tests. I guess that updated versions of the products and the image-optimizer extensions are required, but i can't install their node_modules since access to Sinopia doesn't work within their current state (i was connected to the VPN). As far as i know there where authentication changes last week.
Without those extensions i only have images within the cart right now, but on the majority of our pages i only see placeholders. Refactoring all components to use the featuredImageBaseUrl
instead of the featuredImageUrl
caused a big breaking change. This PWA version (i guess 6.13.0) needs to be deployed with the new extension versions, but there is no protection against a misconfiguration.
What's the plan to handle this?
.setInput(input) | ||
.setVersion(version) | ||
.setInput({ productId }) | ||
.setVersion(3) |
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 would have the same question. It seems that specific versions of the products and the image-optimizer extension needs to be installed. At my current state of the review i can't find any attempt to maintain backwards compatibility, nor protections that the PWA is deployed without those extensions.
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.
there is not backwards compatibility planed. the plan was to have the dependency to the other extensions.
How can we enforce that PWA only gets deployed together with the new versions? Can we use the peerDependencies for this?
@@ -50,9 +52,17 @@ export const isExternal = url => | |||
* @returns {string} | |||
*/ | |||
export const getActualImageSource = (src, { width, height }) => { | |||
if (src && src.includes('images.shopgate.services/v2/images')) { | |||
const { fillColor = 'FFFFFF' } = getThemeSettings('AppImages') || {}; | |||
const format = isAndroidOs ? 'webp' : 'jpeg'; |
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.
Sorry, but i'm not sure, if i get the question? Can you please elaborate it a bit?
* @returns {string} | ||
*/ | ||
export const getActualImageSource = (src, { width, height }) => { | ||
if (src && src.includes('images.shopgate.services/v2/images')) { | ||
const fillColor = 'FFFFFF'; | ||
const format = isAndroidOs ? 'webp' : 'jpeg'; |
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.
This kind of approach to detect webp
support seems to be a bit error-prone. We should try to implement a real feature detection. I found this promising article and did some testing.
Since it uses fetch
which only runs async we would need to postpone rendering of the app till the promise resolves. I archived this by wrapping the render(<Pages store={store} />, document.getElementById('root'))
call within e.g. theme-gmd/index.jsx
with a self invoking asynchronous function which calls render
after the detection was done. You could persist the result of the detection somewhere in the module which provides the detection function to enable synchronous access later.
What do you think?
There was an error on Firefox when running the snippet from the console when no website was open. So i added t try catch block around the fetch
and returned false
in case of an error.
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.
Google devs suggest simpler way of detection: https://developers.google.com/speed/webp/faq#in_your_own_javascript
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 was also thinking about a feature detection, but then decided to keep the old behavior. Would you be fine with moving it to a new ticket? i would like to keep the scope here as small as possible
</div> | ||
); | ||
const Image = ({ product }) => { | ||
const { ListImage: gridResolutions } = getProductImageSettings(); |
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 see that you use this helper function within a couple of functional components. Only two components are traditional React components, one of them can be easily refactored. So this could be a nice candidate for a React hook.
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 agree useProductImages
or useAppImages
as it stands in extension config
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 discussed we will not do the hook in this scope
defaultPrice={props.product.price.default} | ||
specialPrice={props.product.price.special} | ||
const Layout = (props, context) => { | ||
// TODO: use ListImage from AppImages theme config here later as soon as cartItems |
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.
Still TODO
Description
Introduced new theme configs ("AppImages") for images width, height, quality and fillColor.
Added support for images that are not square.
Type of change
Please add an "x" into the option that is relevant:
How to test it
Please describe here any specialty that the tester should be aware of.