-
Notifications
You must be signed in to change notification settings - Fork 220
Conversation
Size Change: -168 kB (10%) 👏 Total Size: 1.57 MB
ℹ️ View Unchanged
|
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.
I did a code only review and this looks fine and clean to me.
const iconClass = [ 'dashicon', 'dashicons-arrow-down-alt2', className ] | ||
.filter( Boolean ) | ||
.join( ' ' ); |
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.
why not use classnames, a dependency we already use (that is quite lightweight).
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.
+1 I have same qu
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.
Because I copy/pasted from gb for this blindly :). I'll add.
return ( | ||
<SVG | ||
xmlns="http://www.w3.org/2000/svg" | ||
viewBox="0 0 20 20" |
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.
not sure how will the icon render, but if we want to keep with Gutenberg new grid system, this should be changed to "-2 -2 24 24"
, but if the icon renders fine and sharp, we should be okay.
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.
Icon seems to be rendering fine, this was copied and pasted directly from the existing dashicons implementation. Since this is mostly temporary (we can replace once we can bump component versions), not sure if we need to change.
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.
Tested here too, couldn't spot anything weird and the size changes are great! 👏
bin/webpack-helpers.js
Outdated
const WebpackRTLPlugin = require( 'webpack-rtl-plugin' ); | ||
const chalk = require( 'chalk' ); | ||
const { omit } = require( 'lodash' ); | ||
const NODE_ENV = process.env.NODE_ENV || 'development'; | ||
|
||
const dashIconReplacmentModule = path.resolve( |
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.
Typo:
const dashIconReplacmentModule = path.resolve( | |
const dashIconReplacementModule = path.resolve( |
const iconClass = [ 'dashicon', 'dashicons-arrow-down-alt2', className ] | ||
.filter( Boolean ) | ||
.join( ' ' ); |
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.
+1 I have same qu
.filter( Boolean ) | ||
.join( ' ' ); | ||
return ( | ||
<SVG |
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.
Not sure if you did or not, but I tested and some very small savings could be made if running the SVG though https://jakearchibald.github.io/svgomg/ - I do this on all icon imports I introduce.
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.
I just copied and pasted directly from the existing dashicons... are the savings enough to warrant implementing?
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.
Ya the difference is super miniscule, not worth the change imo (1byte)
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.
168 bytes → 138 bytes for the X
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.
Really? it was only 1B when I used the tool, maybe I'm using it wrong (169->168B)
* Add 2.7.0 changelog * Add 2.7.0 testing steps * Add #2664 to readme * Update testing notes to add checklist app creation link * Add note that 'No shipping methods placeholder when they are all disabled' requires WC 4.3 * Bumping version strings to new version. * Update readme.txt date Co-authored-by: Darren Ethier <darren@roughsmootheng.in>
Fixes: #2280
As a part of using Notice components from
@wordpress/components
directly for our new notices system, thedashicon
package ended up getting pulled in for anything using notices. This resulted in an increase of ~85kb (unminified) for all builds!While dashicons are removed in later iterations of
@wordpress/components
, we're unable to bump up our dependencies yet because of issues with non-available packages in the versions of WP we support.While we do have some components using dashicons (from the package version we use) the solution is to use the
webpack.NormalModuleReplacementPlugin
to replace anydashicons
imports with a custom version that eliminates the excessive size.To test
This impacts anything loading our notices components. While this includes the new Product Block, because this is blocking 2.7.0, testing should primarily focus on:
Essentially testing involves: