Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

Fix Shopify hosted fonts to load via the local preview URL #1821

Merged
merged 3 commits into from
Dec 7, 2021
Merged

Conversation

karreiro
Copy link
Contributor

@karreiro karreiro commented Dec 3, 2021

WHY are these changes introduced?

Fonts were not loading because the Shopify fonts CDN expects a valid HTTP referer header.

WHAT is this pull request doing?

This PR introduces changes in the ShopifyCLI::Theme::DevServer to request fonts from the CDN with the expected headers and serve them locally.

How to test your changes?

  1. Run shopify theme serve service
  2. Open the http://127.0.0.1:9292
  3. Check the fonts are now loading as expected

Before this PR:
before

After this PR:
after

Post-release steps

None.

Update checklist

  • I've added a CHANGELOG entry for this PR (if the change is public-facing)
  • I've considered possible cross-platform impacts (Mac, Linux, Windows).
  • I've left the version number as is (we'll handle incrementing this when releasing).
  • I've included any post-release steps in the section above.

@karreiro karreiro requested review from macournoyer, a team, hannachen and amcaplan and removed request for a team December 3, 2021 10:16
Copy link
Contributor

@macournoyer macournoyer left a comment

Choose a reason for hiding this comment

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

THIS IS AMAZING! So simple. Thanks for fixing this 👏

@karreiro karreiro merged commit 76038a0 into main Dec 7, 2021
@karreiro karreiro deleted the fix-1533 branch December 7, 2021 08:12
@pinguluk
Copy link

pinguluk commented Dec 9, 2021

Not working for me

@karreiro
Copy link
Contributor Author

karreiro commented Dec 9, 2021

Hi, @pinguluk! I believe it's not working because this fix is not released https://github.com/Shopify/shopify-cli/blob/main/CHANGELOG.md?plain=1#L2-L9 yet. It will be available on our next release soon :)

@pinguluk
Copy link

pinguluk commented Dec 9, 2021

Hi, @pinguluk! I believe it's not working because this fix is not released https://github.com/Shopify/shopify-cli/blob/main/CHANGELOG.md?plain=1#L2-L9 yet. It will be available on our next release soon :)

Hey @karreiro, forgot to mention, that I uninstalled the shopify-cli gem and installed it again via specific_install, that allowed me to install the shopify-cli from this github repo. I know that the update is not yet released officially, but I see that the fix is already merged in the main branch and that's why I installed this verison. I've checked the files locally too and I confirm the changes you've made are there:

Screenshot_355
Screenshot_356

But the fix still doesn't work for me.
Am I missing something?

@karreiro
Copy link
Contributor Author

karreiro commented Dec 9, 2021

Thanks a lot for these extra details, @pinguluk. The fix should work indeed, as you're relying on specific_install.

Could you please share an screenshot similar to this one?

That would allow me to see the HTTP status and the URL of the font we're missing.

Thanks again for the details! :)

@pinguluk
Copy link

pinguluk commented Dec 9, 2021

Thanks a lot for these extra details, @pinguluk. The fix should work indeed, as you're relying on specific_install.

Could you please send me an screenshot like this?

That would allow me to see the HTTP status and the URL of the font we're missing.

Thanks again for the details! :)

Locally:

1
Screenshot_357

2

Screenshot_358

3
Screenshot_359

Live site preview:
4
Screenshot_360

@karreiro
Copy link
Contributor Author

karreiro commented Dec 9, 2021

Thanks for the details, @pinguluk 🎉

I've miss included the suffix /assistant to the ShopifyCLI::Theme::DevServer::CdnFonts class.

I will open a PR fixing it soon! :)

@pinguluk
Copy link

pinguluk commented Dec 9, 2021

Thanks for the details, @pinguluk 🎉

I've miss included the suffix /assistant to the ShopifyCLI::Theme::DevServer::CdnFonts class.

I will open a PR fixing it soon! :)

I confirm that by changing FONTS_CDN = "https://fonts.shopifycdn.com/assistant" to FONTS_CDN = "https://fonts.shopifycdn.com" it now works as intended.

Thank you for your awesome work! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shopify hosted fonts don't load via the local preview url
3 participants