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

Fix makeUrl for Fastly #1039

Merged
merged 8 commits into from
Mar 20, 2019
Merged

Conversation

jimbo
Copy link
Contributor

@jimbo jimbo commented Mar 19, 2019

Description

Fix makeUrl for Fastly image optimization.

Related Issue

Motivation and Context

URLs from GraphQL are now absolute, and they include the media path logic we currently have in makeUrl. So we need to remove this logic and rewrite what remains to be compatible with absolute URLs. Also, we need to enable Fastly image optimization in certain environments.

Verification

  • Ensure category images are loading on the home page
  • Ensure you can soft navigate (click) from home page to a category page
  • Ensure product images are loading in the category gallery
  • Ensure you can soft navigate from a category page to a product page
  • Ensure product images are loading
  • Ensure you can soft navigate to the home page via the header logo

How Has This Been Tested?

  • UAT with the above method, image optimization enabled
  • UAT with the above method, image optimization disabled, Fastly enabled
  • yarn test

@vercel
Copy link

vercel bot commented Mar 19, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.


export default memoize(formatUrl, storeUrlKey);
export default formatUrl;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add memoization back at some point, but it should take useFastly into account.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, it was the memoization. Should have figured that out.

@jimbo jimbo changed the base branch from develop to release/2.1 March 19, 2019 18:29
tjwiebell
tjwiebell previously approved these changes Mar 20, 2019
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran local QA with imageOpt disabled and verified:

  • All images ran through Fastly IO when USE_FASTLY=1
  • Images returned as full size when USE_FASTLY is unset or USE_FASTLY=0

LGTM 👍

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks even better with a descriptive docblock, nice add @sirugh.

@tjwiebell tjwiebell merged commit 6f91632 into release/2.1 Mar 20, 2019
@jimbo jimbo deleted the zetlen/refactor-makeurl-for-fastly branch March 20, 2019 16:09
@sirugh sirugh added version: Minor This changeset includes functionality added in a backwards compatible manner. version: Patch This changeset includes backwards compatible bug fixes. and removed version: Minor This changeset includes functionality added in a backwards compatible manner. labels May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants