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
Removes the star icon font #31670
Removes the star icon font #31670
Conversation
24a454e
to
deaf0a3
Compare
as @peterfabian had pointed out, the shape of the star was not sized and aligned with the original due a different size and center. I fixed by shifting the Y center -45pt and resizing of 101,2% (compared to the "autohint" value resulting after import the font in a larger "slot") |
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.
Hey, thanks for the updated PR!
Do you know about any eot file viewers I could use to check the changes? It seems a bit odd to me that star.eot is just 1.6KB, but WooCommerce.eot increased by 26KB.
Taking a step back here a bit, since WP no longer supports IE, based on this post, it seems we don't need to link to eot file anymore. The eot file is fairly large and since there is no format
hint, the font file will always get downloaded (based on this docs).
I'd say we can go with the "Slightly deeper browser support" here and then we can also remove the SVG and embedded-opentype:
@font-face {
font-family: 'MyWebFont';
src: url('myfont.woff2') format('woff2'),
url('myfont.woff') format('woff'),
url('myfont.ttf') format('truetype');
}
This also means we probably no longer need to change the WooCommerce.eot and SVG file, so only ttf, woff and woff2 needs an update.
Even Twenty Twenty theme from 2 years ago only includes woff2 format, so it should be safe for us to reduce the coverage. What do you think?
Sincerely I haven't find no one that work 😅... I've tested the font on a real page with windows 7 / internet explorer to get sure it works but I didn't realize it had become so big. The reason could be some settings used during the conversion with the linux package called "eot-utils" and, maybe, by changing something I can "compress" it more.
You are right! The eot font is completely useless in 2022 and could be safely removed. Nowdays woff2 is the preferred by most of the browser and woff covers all browsers above ie9. About .ttf I don't think it will ever be used but maybe it can be useful for other purposes. One last thing before i'll update the PR... isn't worth to import the _font.scss file (like here) into all the Twenty-* style? Currently the code @font-face is repeated for each style (the only difference I noticed is the different kind of quotes) but can be imported (and this avoids in the future having to edit all the files as I'm doing now). |
@peterfabian friendly ping, how should we move forward on this one. |
Thanks for the ping, and apologies for dropping the ball here. I'll try to have a look at this within a week or so, but this still feels like a fairly low prio enhancement, so it can take a bit of time until I get back to this. Would you mind rebasing it in the meantime @erikyo please? |
Hi , Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
…dates the template style in order to import from font.scss
Unifies the import of fonts by extending it to the twenty-twenty-three theme
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.
Thanks @erikyo! I left a few comments on a few lines and also have some overarching notes:
- Just like previous reviewers I'm not comfortable with the 290% increase in
WooCommerce.eot
. What options do we have here? E.g., is there an optimization that we can run? - When checking out the branch locally, all the star.* files are still there, and indeed I can't fine a commit that would delete them. Is this intentional? If so, what's the rationale here?
- Can you suggest an application that can view
eot
files? I tried FontForge but it doesn't seem to like these. Before approving these changes I'd like to take a look at all the binary files directly. - For the review instructions, can you list a few pages / views that I can use to verify the fonts still work as intended?
I've edited the font with fontforge that allows to open and export any type of font excluded the eof. For that i converted the font using a linux cli tool... But even I don't know how to read and validate it, there are tools online but they don't read either woo or mine versions of the font.
I would not like to delete the files that are used by third-party plugin, some plugin could rely on that font and I would break the existing stuff. the important thing is that that font is not loaded in the frontend (for this i am loading the same font for the "woocommerce" and "star" font-families
I think in 2024 by now no one will be able to view that kind of font 😅, maybe only ie old versions, but as in the first answer my suggestion is to remove that font (or if you want, please encode the
You can check the icon in the review tab of the single page and below the product photo in the archive. if I remember correctly some widgets also used that font as the "woocommerce/all-reviews". To check correctly, I recommend that you create a font called @font-face {
font-family: 'woo1';
src: url('../fonts/woo1.woff2') format('woff2'),
url('../fonts/woo1.woff') format('woff'),
url('../fonts/woo1.ttf') format('truetype');
font-weight: normal;
font-style: normal;
} And then enable and disable it to be sure that is perfectly of the same shape/size etc thanks! |
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.
Anyhow i want to mention that I don't believe that font is relevant anymore, if the problem is the increased file size I suggest deleting it forever
As even WordPress itself doesn't support Internet Explorer anymore -- the only browser to ever have supported EOT font files -- I'd say we remove the file from WooCommerce. 👍
I would not like to delete the files that are used by third-party plugin, some plugin could rely on that font and I would break the existing stuff. the important thing is that that font is not loaded in the frontend (for this i am loading the same font for the "woocommerce" and "star" font-families
Great points -- agreed. 👍 I still think we should stop using the "star" font family even if we leave it as defined -- a few twenty-*.scss
still reference it. See below.
I tested the changes and everything seems to work well. However, after searching the monorepo for font-family: star
again, I still found SCSS rules that refer to this font family. If we truly want to phase this specialized font family out we should update those as well, don't you think?
I'm also adding @gigitux because there are a few of these references in WooCommerce Blocks and Luigi is listed as the author of at least one of those lines. @gigitux, what do you think -- with the WooCommerce font updated to include the star, can we remove the reference to the "star" font family from WooCommerce Blocks as well?
Peter has moved on to a different team and will not be reviewing this PR.
Yep, you are right! I can still find 11 occurrences of that font-family, and you also correct about some are from the woocommerce-block repository (eg. plugins/woocommerce-blocks/assets/js/atomic/blocks/product-elements/rating-stars/style.scss). If this is what you come up with as well, I will proceed with these substitutions |
@erikyo yep, that's exactly what I'm seeing as well, even without the |
The font family 'star' was replaced in all files. However, it continues to remain physically in the repo but not in the style files
I did a final check and realised that in some themes (twenty-nineteen.scss,twenty-nineteen.scss,woocommerce.scss) the font-family was enclosed in quotes, so I replaced those as well. The only remaining occurrences are in the css (generated, I won't change it), I can't find that font anywhere else now. |
Thanks, @erikyo, for working on this!
I can confirm that everything works okay on WooCommerce Blocks:
I agree with this! Let's remove .oet font given that any modern browser supports it (TIL): Also, it would be great if you could update the testing instructions according to this documentation! |
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.
Took another look at the changes and tried things out. In my testing everything still works as intended. Let's ship this. 🚢
All Submissions:
Changes proposed in this Pull Request:
This Pr will replace the WooCommerce font with a new one that is the merge of the previous WooCommerce Font and the Star font. In this way we need to load only a single font to get the same result.
Closes #31490
How to test the changes in this Pull Request:
Changelog entry
Significance
Type
Message
Removes star font
Comment
Testing Instructions
function.php
file:thanks!