Skip to content

Conversation

BPScott
Copy link
Member

@BPScott BPScott commented Feb 7, 2019

WHY are these changes introduced?

This adds treeshaking support, esm builds and has renamed the exports.

WHAT is this pull request doing?

  • Update to @shopify/polaris-icons v2.0.0
  • Update imports to reflect new export names

Note that the names you put into Icon source remain unaffected - you'd still do <Icon source="add">, but that now add maps to the named export addMinor.

How to 🎩

  • Assert percy passes (and thus icons still render)
  • Assert build passes (and thus there's no incorrect imports)

@BPScott BPScott requested a review from amrocha February 7, 2019 00:12
@BPScott BPScott temporarily deployed to polaris-react-pr-982 February 7, 2019 00:12 Inactive
@@ -1,54 +1,54 @@
import POLARIS_ICONS from '@shopify/polaris-icons';
Copy link
Member Author

Choose a reason for hiding this comment

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

This whole file could all be replaced with just export * from '@shopify/polaris-icons' but IIRC @amrocha and I decided that it's best to be explicit.

I'm still kinda tempted to 🔥 this file entirely and just import icons direct within the Icon component but we can decide if we want to do that later

Copy link
Contributor

Choose a reason for hiding this comment

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

Like we discussed, I believe having an index file creates good habits. Prune a list of icons down to the ones you need and use them, instead of potentially adding a new icon every time

On the export * side though, I talked to Kaelig and it sounds like there's only a few icons that are problematic, so I'm gonna put something together soon, run it by Jesse and see if we can get it merged. If we do then what I said above doesn't apply anymore and we can get rid of this file

This adds treeshaking support, esm builds and has renamed the exports.
@@ -1,54 +1,54 @@
import POLARIS_ICONS from '@shopify/polaris-icons';
Copy link
Contributor

Choose a reason for hiding this comment

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

Like we discussed, I believe having an index file creates good habits. Prune a list of icons down to the ones you need and use them, instead of potentially adding a new icon every time

On the export * side though, I talked to Kaelig and it sounds like there's only a few icons that are problematic, so I'm gonna put something together soon, run it by Jesse and see if we can get it merged. If we do then what I said above doesn't apply anymore and we can get rid of this file

@BPScott BPScott merged commit f8e8f22 into master Feb 7, 2019
@BPScott BPScott deleted the polaris-icons-2.0 branch February 7, 2019 00:48
@danrosenthal danrosenthal temporarily deployed to production February 11, 2019 14:35 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants