Skip to content

Comment Likes: Remove the dependency on Noticons#14784

Merged
kraftbj merged 1 commit intomasterfrom
update/comment-likes-inline-icon
Mar 2, 2020
Merged

Comment Likes: Remove the dependency on Noticons#14784
kraftbj merged 1 commit intomasterfrom
update/comment-likes-inline-icon

Conversation

@dero
Copy link
Copy Markdown
Contributor

@dero dero commented Feb 24, 2020

Currently the Noticons font is loaded solely for the purpose
of displaying the "like" star from the Noticons icon set. This
adds 20 kB to the total page load size when only comment likes
are enabled.

This commit replaces the icon character by an inline SVG used
on WPCOM.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • This updates the likes module and affects comment like buttons.

Testing instructions:

  • Go to Jetpack Settings > Discussion and Enable comment likes.
  • Load a single post page with comments displayed.
  • Scroll down to view the comments.
  • Refresh and observe the "Loading..." placeholder having a blue star displayed left to it.
  • In the browser dev tools, notice the Noticons font is not being loaded.

Proposed changelog entry

  • Inline the single placeholder icon used by Comment Likes to avoid loading a whole icon font.

Concern

I recognize that hardcoding the inline image value into the stylesheet is not a great solution. Is there a tool in the build pipeline that would be able to automate the task, i.e. fetch the glyph from Noticons and inline it in the generated CSS?

@dero dero added the [Status] Needs Review This PR is ready for review. label Feb 24, 2020
@dero dero requested a review from a team February 24, 2020 17:40
Currently the Noticons font is loaded solely for the purpose
of displaying the "like" star from the Noticons icon set. This
adds 20 kB to the total page load size when only comment likes
are enabled.

This commit replaces the icon character by an inline SVG used
on WPCOM.
@dero dero force-pushed the update/comment-likes-inline-icon branch from 0b1573f to d5ef47c Compare February 24, 2020 17:42
@jetpackbot
Copy link
Copy Markdown
Collaborator

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against d5ef47c

@matticbot
Copy link
Copy Markdown
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello dero! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D39719-code before merging this PR. Thank you!

@kraftbj kraftbj added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 2, 2020
@kraftbj kraftbj merged commit 5438ee9 into master Mar 2, 2020
@kraftbj kraftbj deleted the update/comment-likes-inline-icon branch March 2, 2020 22:51
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 2, 2020
jeherve added a commit that referenced this pull request Mar 23, 2020
jeherve added a commit that referenced this pull request Mar 31, 2020
* Initial changelog entry

* Changelog: add #14904

* Changelog: add #14910

* Changelog: add #14913

* Changelog: add #14916

* Changelog: add #14922

* Changelog: add #14924

* Changelog: add #14925

* Changelog: add #14928

* Changelog: add #14840

* Changelog: add #14841

* Changelog: add #14842

* Changelog: add #14826

* Changelog: add #14835

* Changelog: add #14859

* Changelog: add #14884

* Changelog: add #14888

* Changelog: add #14817

* Changelog: add #14814

* Changelog: add #14819

* Changelog;: add #14797

* Changelog: add #14798

* Changelog: add #14802

* Changelog: add #13676

* Changelog: add #13744

* Changelog: add #13777

* Changelog: add #14446

* Changelog: add #14739

* Changelog: add #14770

* Changelog: add #14784

* Changelog: add #14897

* Changelog: add #14898

* Changelog: add #14968

* Changelog: add #14985

* Changelog: add #15044

* Changelog: add #15052

* Update to remove Podcast since it remains in Beta

* Changelog: add #14803

* Changelog: add #15028

* Changelog: add #15065

* Changelog:add #14886

* Changelog: add #15118

* Changelog: add #14990

* Changelog: add #14528

* Changelog: add #15120

* Changelog: add #15126

* Changelog: add #15049

* Chanegelog: add #14852

* Changelog: add #15090

* Changelog: add #15138

* Changelog: add #15124

* Changelog:add #15055

* Changelog: add #15017

* Changelog: add #15109

* Changelog: add #15145

* Changelog:add #15096

* Changelog:add #15153

* Changelog: add #15133

* Changelog: add #14960

* Changelog: add #15127

* Changelog: add #15056

* Copy current changelog to changelog archive.

* Clarify changelog description
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants