add new icons and remove old ones #1644

senadir · 2020-01-24T13:12:58Z

This is a large PR, but it should be simple to review since all of those changes are manual.

This PR uses the new icons provided in #1418, it also updates other icons to the more consistent ones found in Material Design.

It uses the same mythology for including incudes and loading them as presented in WordPress/gutenberg#17055.

Why `<Icon icon={ name } />` and not `<IconName />`

There is no strong preference, but I believe having a shared component could be good for any feature modifications, it’s also more readable.

Why a separate folder and not keeping them in components

No strong preference in this one as well, I felt having a separate package for icons more preferable since I don’t count them as components, it might provide more utility in the future, like sharing them.

Why `<SVG>` and not `<svg>`

<SVG> offer some accessibility features like area-hidden, is-pressed and on so on, that is already used in Gutenberg.

Naming

The previous naming of icons was inconsistent, sometimes icons are named based on the block using them, this PR switches the naming to a function naming (I named them according to what they look like) open to suggestions here.

closes #1307
closes #1418
fixes #1577

How to test the changes in this Pull Request:

Compare icons used with ones in Change WooCommerce block icons #1418.
Compare icons in used inside blocks controls.

Changelog

Update the icons used in the blocks.

Aljullu

Wow, great job with this and WordPress/gutenberg#17055, @senadir. I like how both pulls go in the same direction.

I left some minor comments below, but this is overall looking good to me. 👍

assets/js/blocks/featured-category/block.js

assets/js/icons/README.md

assets/js/icons/index.js

assets/js/icons/README.md

assets/js/blocks/products-by-attribute/block.js

nerrad

Awesome job here!

I like the decision to have a standard <Icon/> component that you feed the actual icon to. It standardizes the api. As noted in an inline comment, I'm not sure I like the name icon for the prop as it still seems a bit ambiguous to me. However, I do get that it is what got merged to the @wordpress/icons package so if there's disagreement, we can roll with that example.
I agree this would be good to eventually publish as a package (and why the separate folder). Big 👍 on implementing as an alias in the meantime! Since all woo packages will be published from the wc-admin repo, we should look into eventually moving this to the wc-admin repo and publishing from there (maybe do on the meetup). Once @wordpress/icons is available/published, we should probably look into extending that instead - but the nice thing here is so far the interface is similar so doing so shouldn't be too hard.
Agree on SVG instead of svg (only concern I had was WP version support, but I tested this branch on WP 5.0 and it works so all good there).
Re, naming of icons: I'm okay with the name changes (aside from a couple comments pointed out by Albert). The nice thing is if someone doesn't like it, they can always alias on the import. This will be a good library to have storybook support when that is implemented so we can review the icons.
It looks like these changes increases the asset size of built bundles (npm run build) for some of the blocks. It seems minor (mostly 1kb bumps) but something to be aware of. I wonder if this is just because of some tweaks in how the icons are loaded (potential svg size changes and maybe extra code?). I thought there might be more tree-shaking involved here that would reduce the sizes but doesn't look like it.

Here's a screenshot of all the new block icons added:

I think it might be good to get this signed off by design before merging since there are a number of changes here. cc @garymurray

assets/js/blocks/featured-category/block.js

assets/js/icons/README.md

assets/js/icons/icon/index.js

assets/js/icons/index.js

package.json

garymurray · 2020-01-24T16:28:51Z

I think it might be good to get this signed off by design before merging since there are a number of changes here. cc @garymurray

LGTM 🚢

senadir · 2020-01-27T12:11:44Z

It looks like these changes increases the asset size of built bundles (npm run build) for some of the blocks. It seems minor (mostly 1kb bumps) but something to be aware of. I wonder if this is just because of some tweaks in how the icons are loaded (potential svg size changes and maybe extra code?). I thought there might be more tree-shaking involved here that would reduce the sizes but doesn't look like it.

there are no changes to the frontend as far as I'm aware, all changes now target editor components, those used to use dash icon which was an external of wp.comopnents, now we ship our own icons, each icon is 300-500 byte (raw, no compress), changes are minimal, we might start to see value once we ship icons to the frontend and once new changes ship to npm for the wp.components many components we use on the frontend like Card and Panel and PanelBody won't be using Dashicon, so we should save 80kb here and there.

Gridicon could have been loaded with wc-vendors or already loaded, it was never included in our builds directly, so saving there are not straightforward

mikejolley · 2020-01-27T16:15:16Z

@senadir Question: I saw each icon name is lowercase - should those components be capitalised? (e.g. bill rather than Bill).

r/e #1644 (comment) svg made sense to me, for clarity.

nerrad · 2020-01-27T16:27:17Z

should those components be capitalised? (e.g. bill rather than Bill).

I think the difference here is that these are react nodes vs components. So the Icon component is receiving an instance of <SVG /> not const Cart = ( { props } ) => <SVG { ...props } />. That's why they can be lowercase here.

nerrad · 2020-01-27T16:29:50Z

assets/js/icons/icon/index.js

+	return cloneElement( srcElement, {
+		width: size,
+		height: size,
+		...props,
+	} );


I wonder if we should check that srcElement is a valid react element before using (you can use isElement from the @wordpress/element package for this).

nerrad · 2020-01-27T16:32:10Z

r/e #1644 (comment) svg made sense to me, for clarity.

Initially I agree, however do we want to allow for submitting non svg images to serve as the icon? Or do we want to specifically restrict to SVG nodes only? If the latter, then I wonder if we should explicitly test for whether the provided node is SVG (you can use isElementOfType for this).

senadir · 2020-01-27T16:32:30Z

Question: I saw each icon name is lowercase - should those components be capitalised? (e.g. bill rather than Bill).

+1 to what Darren said, also since we're passing direct elements, we gain a small performance boost, and with cloneElement, we can replace children props, so even if our icons already have props defined like width and height and viewbox, we can replace those in <Icon />. However, this is only possible when you know for sure you have 1 child

senadir · 2020-01-27T16:36:01Z

Initially I agree, however do we want to allow for submitting non svg images to serve as the icon? Or do we want to specifically restrict to SVG nodes only? If the latter, then I wonder if we should explicitly test for whether the provided node is SVG?

Can you think of a scenario in which we might not pass an SVG? I think restricting is good, using propTypes as you suggested

nerrad · 2020-01-27T16:36:29Z

Can you think of a scenario in which we might not pass an SVG?

passing <img />.

- icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals)

senadir · 2020-01-30T13:21:26Z

@nerrad I've left it to accept any element (images or svgs), added isValidElement to it, I think this is ready for a merge?

nerrad

I have a couple questions still but they are non-blocking. You decide whether it warrants additional commits or not. Great work!

assets/js/blocks/featured-category/block.js

assets/js/icons/icon/index.js

- icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals)

* install & configure storybook (via magic npx script) * fix indentation in storybook generated files * eslint ignore generated storybook files (for now at least) * unhide storybook folder, consistent with Gutenberg project * demo story for one of our components (with no css/styles) * hack in scss webpack config & add story for button: - fixes scss imports breaking storybook build - note scss / styling doesn't work yet + organise our component stories into folder * git ignore storybook-static build folder * pin dependencies for storybook * piggy-back off main webpack config for storybook module.rules (for scss) * use gutenberg (wp-components) styles in storybook * use system font for storybook, consistent with wp-admin/gberg and reasonable default for components in front end * add --ci flag to prevent storybook opening new browser tab… - see also storybookjs/storybook#6201 * rename default stories to Default (following Gutenberg pattern) * add story for ErrorPlaceholder * failing ProductPreview story (committing to PR as an example for discussion) * storybook for components/icons * fix aliased dependencies in components for storybook: append our webpack aliases to storybook webpack config * basic story for PriceSlider (looks right but interaction broken) * fix PriceSlider user interaction: - PriceSlider expects client to handle onChange and pass in new min/max * add comment about priceslider max/min (todoish) * remove default stories from storybook scaffolding * organise stories by module (aka folder in codebase) * package-lock update after rebase * remove unnecessary ignores (default stories are gone) * delete experimental/risky/broken stories: - icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals) * remove unnecessary import * clarify PriceSlider component intended usage comment in story * remove redundant wrapper divs from stories * add common storybook addons (used by Gutenberg storybook) * rebuild package.lock after rebase * remove unnecessary wrapper div * package fixes after rebase * add configuration for storybook source loader * add decorators for a11y and knobs plugins * remove unnecessary react import & import useState from WP Co-authored-by: Darren Ethier <darren@roughsmootheng.in>

nerrad

Let's 🎉 🌮!

senadir added the status: needs review label Jan 24, 2020

senadir requested a review from a team January 24, 2020 13:12

senadir self-assigned this Jan 24, 2020

Aljullu reviewed Jan 24, 2020

View reviewed changes

nerrad reviewed Jan 24, 2020

View reviewed changes

senadir requested a review from a team as a code owner January 27, 2020 12:07

senadir requested review from mikejolley and removed request for a team January 27, 2020 12:07

senadir requested a review from nerrad January 27, 2020 12:13

nerrad added this to the 2.6.0 milestone Jan 27, 2020

nerrad reviewed Jan 27, 2020

View reviewed changes

nerrad mentioned this pull request Jan 28, 2020

Basic storybook implementation #1636

Merged

haszari added a commit that referenced this pull request Jan 30, 2020

delete experimental/risky/broken stories:

ddbf284

- icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals)

haszari added a commit that referenced this pull request Jan 30, 2020

delete experimental/risky/broken stories:

0cc5305

- icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals)

senadir force-pushed the add/new-icons branch from f4c4ec5 to e4e5884 Compare January 30, 2020 13:22

senadir added 6 commits January 30, 2020 14:23

add new icons and remove old ones

57efb36

add docs

bba4a11

add notes and remove gridicons

10d975e

update to folderStarred

539fef2

typos

b3a487b

reorder imports

d52b932

senadir added 3 commits January 30, 2020 14:24

update prop name to srcElement

ecc02bb

validate element

0509a6c

fix bad import

339beec

senadir force-pushed the add/new-icons branch from e4e5884 to 339beec Compare January 30, 2020 13:24

nerrad approved these changes Jan 30, 2020

View reviewed changes

assets/js/blocks/featured-category/block.js Outdated Show resolved Hide resolved

assets/js/icons/icon/index.js Show resolved Hide resolved

woocommercebot added status: ready to merge and removed status: needs review labels Jan 30, 2020

haszari added a commit that referenced this pull request Jan 30, 2020

delete experimental/risky/broken stories:

b2c9bda

- icons components are changing in #1644 - we need to refactor/do more work to get ProductPreview working (settings globals)

senadir added 2 commits January 31, 2020 16:15

lowecase folderStarred

16a1753

add propTypes

fd36b80

nerrad approved these changes Jan 31, 2020

View reviewed changes

senadir merged commit 20fcd2e into master Jan 31, 2020

senadir deleted the add/new-icons branch January 31, 2020 18:20

senadir mentioned this pull request Feb 14, 2020

Change PropType validation for Icon component #1737

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new icons and remove old ones #1644

add new icons and remove old ones #1644

senadir commented Jan 24, 2020 •

edited

Loading

Aljullu left a comment

nerrad left a comment •

edited

Loading

garymurray commented Jan 24, 2020

senadir commented Jan 27, 2020 •

edited

Loading

mikejolley commented Jan 27, 2020

nerrad commented Jan 27, 2020

nerrad Jan 27, 2020

nerrad commented Jan 27, 2020 •

edited

Loading

senadir commented Jan 27, 2020 •

edited

Loading

senadir commented Jan 27, 2020

nerrad commented Jan 27, 2020 •

edited

Loading

senadir commented Jan 30, 2020

nerrad left a comment

nerrad left a comment

add new icons and remove old ones #1644

add new icons and remove old ones #1644

Conversation

senadir commented Jan 24, 2020 • edited Loading

Why <Icon icon={ name } /> and not <IconName />

Why a separate folder and not keeping them in components

Why <SVG> and not <svg>

Naming

How to test the changes in this Pull Request:

Changelog

Aljullu left a comment

Choose a reason for hiding this comment

nerrad left a comment • edited Loading

Choose a reason for hiding this comment

garymurray commented Jan 24, 2020

senadir commented Jan 27, 2020 • edited Loading

mikejolley commented Jan 27, 2020

nerrad commented Jan 27, 2020

nerrad Jan 27, 2020

Choose a reason for hiding this comment

nerrad commented Jan 27, 2020 • edited Loading

senadir commented Jan 27, 2020 • edited Loading

senadir commented Jan 27, 2020

nerrad commented Jan 27, 2020 • edited Loading

senadir commented Jan 30, 2020

nerrad left a comment

Choose a reason for hiding this comment

nerrad left a comment

Choose a reason for hiding this comment

senadir commented Jan 24, 2020 •

edited

Loading

Why `<Icon icon={ name } />` and not `<IconName />`

Why `<SVG>` and not `<svg>`

nerrad left a comment •

edited

Loading

senadir commented Jan 27, 2020 •

edited

Loading

nerrad commented Jan 27, 2020 •

edited

Loading

senadir commented Jan 27, 2020 •

edited

Loading

nerrad commented Jan 27, 2020 •

edited

Loading