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

Manage OFN's font through the asset pipeline #5644

Conversation

sauloperez
Copy link
Contributor

@sauloperez sauloperez commented Jun 19, 2020

What? Why?

Related to #5643

This file contains the various icons we use in the UI, such as all the icons shown in the dropdowns in the front-office.

Moves this file to the assets folder (away from public/) and references its fingerprinted version.

What should we test?

All the icons should be shown as usual. You can look them up by ofn-i_ in the HTML. It's used as the class attribute.

This needs to be checked across browsers in Browsterstack, mainly the latest version of IE and Edge.

Release notes

Reference OFN's custom font using its fingerprinted path

Changelog Category: Changed

@sauloperez sauloperez self-assigned this Jun 19, 2020
@sauloperez sauloperez force-pushed the manage-font-with-asset-pipeline branch from b3c7abe to 2e4a8a5 Compare June 19, 2020 13:38
@sauloperez sauloperez marked this pull request as ready for review June 19, 2020 13:53
Copy link
Contributor

@luisramos0 luisramos0 left a comment

Choose a reason for hiding this comment

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

nice 👍

src: font-url('OFN-v2.eot');
src: font-url('OFN-v2.eot') format('embedded-opentype'),
font-url('OFN-v2.woff') format('woff'),
font-url('OFN-v2.ttf') format('truetype'),
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice the previous versions have suffixes added, but you've removed them. Are they not needed? Eg:

'/OFN-v2.eot?#iefixeslsji' is now: 'OFN-v2.eot'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I strongly believe that was the pre-asset-pipeline poor-man's asset versioning. That's what the fringerprint came to solve way better. Unless iefixes refers to IE? Googling yielded no results though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be fixes for IE, yes. I have a feeling the multiple file formats are there because they can't all be rendered in all browsers... Maybe we could have a look using Browserstack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I added it in the test instructions

@@ -13,7 +13,7 @@
- else
= favicon_link_tag "/favicon-staging.ico"
%link{href: "https://fonts.googleapis.com/css?family=Roboto:400,300italic,400italic,300,700,700italic|Oswald:300,400,700", rel: "stylesheet", type: "text/css"}
%link{href: "/OFN-v2.woff?eslsji", rel: "preload", as: "font", crossorigin: "anonymous"}
%link{href: font_path("OFN-v2.woff"), rel: "preload", as: "font", crossorigin: "anonymous"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Bonus points for keeping the font-preloading suggested by PageSpeed Insights

💯

Copy link
Contributor

@Matt-Yorkley Matt-Yorkley left a comment

Choose a reason for hiding this comment

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

I left a comment about the font file suffixes. I think they're related to cross-browser compatibility, but I'm not sure about them.

Feel free to move it to test-ready if you think it's okay as-is 👍

@sauloperez
Copy link
Contributor Author

Let's see what testing says.

@sauloperez sauloperez force-pushed the manage-font-with-asset-pipeline branch from 2e4a8a5 to 0c93de8 Compare June 30, 2020 07:02
@filipefurtad0 filipefurtad0 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jul 1, 2020
@filipefurtad0 filipefurtad0 self-assigned this Jul 2, 2020
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jul 2, 2020
@filipefurtad0
Copy link
Contributor

Hey @sauloperez ,

I checked the homepage, where some of these fonts appear. It looks good on my system (Ubuntu Firefox/Chrome), mobile (android/iPhone), and in Windows 10 IE/Edge:

Browserstack - Windows 10 - IE 11:
image

Browserstack - Windows 10 - Edge 83:
image

Good to go!

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jul 2, 2020
@luisramos0 luisramos0 merged commit 5104495 into openfoodfoundation:master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants