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

Remove redundant font-face declaration from font CSS template #360

Merged
merged 1 commit into from
May 14, 2019

Conversation

aduth
Copy link
Member

@aduth aduth commented May 8, 2019

This pull request seeks to remove a redundant @font-face declaration from the font CSS template.

It appears to have been introduced with #246, though it is not entirely clear to me from where it originated. Earlier versions of the default template from grunt-webfont included similar duplication, but this was later removed in v1.2.0 (March 2016):

sapegin/grunt-webfont@5299f94#diff-391cdceb80b08038931553dfae720a89

The changes proposed here effectively mimic the changes from the above pull request.

Why

  • The @font-face are effectively duplicate, both for the unstyled, default-weighted font of the same family name.
  • The remaining @font-face still respects browser support for old (read: unsupported) versions of Internet Explorer (reference)
  • Initial testing shows it to be a remedy for the issue described at https://core.trac.wordpress.org/ticket/47183
  • Arguably the EOT format could be dropped altogether, based on project browser support (EOT exists for compatibility of Internet Explorer 9 and lower)

@aduth aduth added the Build label May 8, 2019
@aduth aduth requested a review from jasmussen May 8, 2019 15:27
@aduth
Copy link
Member Author

aduth commented May 8, 2019

Note: I was observing many more changes than expected when running grunt in this branch, which seemed to me to be unrelated to the specific changes. As such, I only committed the expected changes.

The changes can be seen on the master branch as well:

git checkout master
grunt
⇒ git status   
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   icon-font/css/dashicons.css
	modified:   icon-font/dashicons.html
	modified:   icon-font/fonts/dashicons.eot
	modified:   icon-font/fonts/dashicons.ttf
	modified:   icon-font/fonts/dashicons.woff2

no changes added to commit (use "git add" and/or "git commit -a")

@jasmussen
Copy link
Collaborator

Note: I was observing many more changes than expected when running grunt in this branch, which seemed to me to be unrelated to the specific changes. As such, I only committed the expected changes.

I think this is due to a setting that is intended to bust caching. But I'm not convinced. (Also sidenote, I run npm run build instead of grunt, though I get the same result). CC @desrosj for a sanity check: is the dashicons repo ahead or behind what's shipping with core now?

Copy link
Collaborator

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Interesting, very solid catch. When I first saw this review request, at a glance I assumed that the duplicate syntax referred to the "bulletproof font face syntax", which is an elaborate hack to serve the right fonts to the right browsers: https://www.paulirish.com/2009/bulletproof-font-face-implementation-syntax/ — reading the actual syntax looks very much duplicate.

However yeah, this just looks to be a duplicate.

To be sure I tested this in IE as well. All's good:

Screenshot 2019-05-09 at 09 40 32

And Edge:

Screenshot 2019-05-09 at 09 36 37

@desrosj desrosj self-requested a review May 14, 2019 15:00
@desrosj
Copy link
Collaborator

desrosj commented May 14, 2019

Looks good!

@desrosj desrosj merged commit 3136c3d into master May 14, 2019
@desrosj desrosj deleted the remove/redundant-font-face branch May 14, 2019 15:02
@desrosj
Copy link
Collaborator

desrosj commented May 15, 2019

I think this is due to a setting that is intended to bust caching. But I'm not convinced.

Just realized I never answered this above. My understanding is that every time a build is run, the string used for cache busting is changed, even if there are no actual changes present. This results in changes to the CSS and HTML file every time. Though, I am not sure why the actual files themselves are changed, though. It could be there are some file properties that are changed.

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

Successfully merging this pull request may close these issues.

3 participants