Skip to content
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

'small_image' in 'ProductInterface' does not have enough info #88

Closed
DrewML opened this Issue Jun 6, 2018 · 5 comments

Comments

Projects
None yet
8 participants
@DrewML
Copy link
Member

DrewML commented Jun 6, 2018

Preconditions

  1. 2.3-develop branch

Steps to reproduce

  1. Using /graphql, POST a query that requests the small_image for a product
  2. The string returned for the small_image field will be in the format /m/b/mb02-gray-0.jpg.
  3. The format returned does not have enough information for a client-rendered Magento application to append the uri to an img tag. The returned format is https://mmnl.test/media/catalog/product/cache/2765542505660baab28ecd555e27366e/m/b/mb02-gray-0.jpg

Expected result

  1. small_image (and other image values in the ProductInterface) should return a URL that can be used as-is to fetch the associated image

Actual result

  1. small_image is populated with a value that can not be used without obtaining other info about cache configuration.

AC:

@misha-kotov misha-kotov changed the title `small_image` in `ProductInterface` does not have enough info 'small_image' in 'ProductInterface' does not have enough info Jun 6, 2018

@paliarush paliarush self-assigned this Jun 11, 2018

zetlen added a commit to magento-research/pwa-studio that referenced this issue Jun 26, 2018

feat(data): Add live GraphQL data to product detail page
Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

 - Added GraphQL query to
 `packages/venia-concept/src/RootComponents/Product.js`.
     - Resolves from URL by using the `url_key` in a GraphQL query.
     - Can also resolve by SKU.
     - Modified prop types and render method to accommodate live data shape.
 - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type.
    - Uses the `window.Intl` standard object to format.
 - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape.
 - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic.
 - Created a shared `propShapes.js` file containing commonly used prop type expressions.
 - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function.
 - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs.
   - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.**
   - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`.
   - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`.
   - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance.

 - Optimize queries with fragments
 - Centralize queries in query file to be preprocessed
 - Make link to product detail on category page
 - Resolve media URL issue
 - Test with image galleries

Closes #87.

zetlen added a commit to magento-research/pwa-studio that referenced this issue Jun 26, 2018

feat(data): Add live GraphQL data to product detail page
Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

 - Added GraphQL query to
 `packages/venia-concept/src/RootComponents/Product.js`.
     - Resolves from URL by using the `url_key` in a GraphQL query.
     - Can also resolve by SKU.
     - Modified prop types and render method to accommodate live data shape.
 - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type.
    - Uses the `window.Intl` standard object to format.
 - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape.
 - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic.
 - Created a shared `propShapes.js` file containing commonly used prop type expressions.
 - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function.
 - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs.
   - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.**
   - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`.
   - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`.
   - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance.

 - Optimize queries with fragments
 - Centralize queries in query file to be preprocessed
 - Make link to product detail on category page
 - Resolve media URL issue
 - Test with image galleries

Closes #87.

@paliarush paliarush removed their assignment Jun 27, 2018

@dmytro-ch dmytro-ch self-assigned this Jun 30, 2018

@jissereitsma

This comment has been minimized.

Copy link
Contributor

jissereitsma commented Jul 6, 2018

Perhaps there simply needs to come a second property small_image_url because you could argue that the image stays the same, while the URL might change depending on media URL settings. The problem here is that the GraphQL schema refers to the generic ProductInterface of the catalog module, which counts as @api. I've tried to fix things there, but that leads to a lot of work.

An easier solution would simply be to use a variable media_url within PWA? What I've now done for myself is create a new file src/config.json with a JSON string {"media_url": "/media/catalog/product/"}. Next, in the component where a real URL is needed, I import this JSON (import config from 'src/config.json';) and I construct the URL itself using config.media_url + small_image. Works for me.

zetlen added a commit to magento-research/pwa-studio that referenced this issue Jul 6, 2018

feat(data): Add live GraphQL data to product detail page
Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

 - Added GraphQL query to
 `packages/venia-concept/src/RootComponents/Product.js`.
     - Resolves from URL by using the `url_key` in a GraphQL query.
     - Can also resolve by SKU.
     - Modified prop types and render method to accommodate live data shape.
 - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type.
    - Uses the `window.Intl` standard object to format.
 - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape.
 - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic.
 - Created a shared `propShapes.js` file containing commonly used prop type expressions.
 - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function.
 - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs.
   - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.**
   - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`.
   - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`.
   - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance.

 - Optimize queries with fragments
 - Centralize queries in query file to be preprocessed
 - Make link to product detail on category page
 - Resolve media URL issue
 - Test with image galleries

Closes #87.
@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Jul 6, 2018

An easier solution would simply be to use a variable media_url within PWA? What I've now done for myself is create a new file src/config.json with a JSON string {"media_url": "/media/catalog/product/"}. Next, in the component where a real URL is needed, I import this JSON (import config from 'src/config.json';) and I construct the URL itself using config.media_url + small_image. Works for me.

There is a problem here with portability. If I'm writing a component for any PWA theme to be able to consume, I'd like to not require that every consumer always pass me a media_url, and I can't import it from a static JSON file, because my component is not part of a specific theme.

The other issue is just duplication. If this value already exists on the backend, it wouldn't be great to duplicate that in front-end configuration. We then have an easy place for configs to mismatch and lead to hard-to-debug 404s.

Another thing to keep in mind, too, is that this API is not just for PWA - it's for every consumer of Magento APIs over the next (however many) years. Consuming the API should require as little knowledge of Magento's image management as possible. The more intuitive the API is for new consumers, the better!

@jissereitsma

This comment has been minimized.

Copy link
Contributor

jissereitsma commented Jul 7, 2018

@DrewML Completely get it. Testing such a component would also point out the flaw - it is another dependency. So it should be in the GraphQL API. However, I do not think it should be in the small_image property itself. The image might remain the same, while the URL to that image might be different (CDN, media database). A property small_image_url sounds more adequate.

@DrewML

This comment has been minimized.

Copy link
Member Author

DrewML commented Jul 7, 2018

@jissereitsma that sounds reasonable to me. You'd know better than I would what the right call is here 🙂

zetlen added a commit to magento-research/pwa-studio that referenced this issue Jul 9, 2018

feat(data): Add live GraphQL data to product detail page
Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

 - Added GraphQL query to
 `packages/venia-concept/src/RootComponents/Product.js`.
     - Resolves from URL by using the `url_key` in a GraphQL query.
     - Can also resolve by SKU.
     - Modified prop types and render method to accommodate live data shape.
 - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type.
    - Uses the `window.Intl` standard object to format.
 - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape.
 - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic.
 - Created a shared `propShapes.js` file containing commonly used prop type expressions.
 - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function.
 - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs.
   - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.**
   - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`.
   - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`.
   - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance.

 - Optimize queries with fragments
 - Centralize queries in query file to be preprocessed
 - Make link to product detail on category page
 - Resolve media URL issue
 - Test with image galleries

Closes #87.

zetlen added a commit to magento-research/pwa-studio that referenced this issue Jul 9, 2018

feat(data): Add live GraphQL data to product detail page (#90)
Basic implementation of a GraphQL query for product details. Builds on #52 by replicating the inline query declaration. Had to plumb out the child components for the new data shape; in doing so, I made a few reusable functions.

 - Added GraphQL query to
 `packages/venia-concept/src/RootComponents/Product.js`.
     - Resolves from URL by using the `url_key` in a GraphQL query.
     - Can also resolve by SKU.
     - Modified prop types and render method to accommodate live data shape.
 - Added `<Currency />` component whose signature matches the Magento GraphQl `Money` type.
    - Uses the `window.Intl` standard object to format.
 - Modified the `Gallery` and `ProductImageCarousel` components to use new data shape.
 - Moved shared constant data URIs to a single `src/shared` folder, to replicate placeholder logic.
 - Created a shared `propShapes.js` file containing commonly used prop type expressions.
 - Anticipating that `url_key` would be a common way to navigate, I made a `url_key` utility function.
 - Added a `makeProductMediaPath` utility function, for turning product image file paths from API responses into relative URLs.
   - Though [magento/graphql-ce/issues/88](magento/graphql-ce#88) is still a problem for production, I found that **when `magento-sample-data` is installed, it symlinks into the `pub/media` folder so you can use simpler URLs.**
   - You can see this for yourself with `ls -l <magento-root>/pub/media/catalog/product`.
   - So I added a `makePathPrepender` function, which we'll later use often, that can create functions like `makeProductMediaPath`.
   - I hardcoded `/media/catalog/products` in the code, but I also added an environment variable to `.env` and `webpack.config.js` for configuring that URL per instance.

 - Optimize queries with fragments
 - Centralize queries in query file to be preprocessed
 - Make link to product detail on category page
 - Resolve media URL issue
 - Test with image galleries

Closes #87.

@dmytro-ch dmytro-ch removed their assignment Jul 12, 2018

@rogyar rogyar self-assigned this Jul 18, 2018

@rogyar rogyar added the in-progress label Jul 18, 2018

@naydav

This comment has been minimized.

Copy link
Contributor

naydav commented Sep 26, 2018

@naydav naydav closed this Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.