Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spark)!: remove all Icons #134

Merged
merged 11 commits into from
Jul 15, 2021
Merged

feat(spark)!: remove all Icons #134

merged 11 commits into from
Jul 15, 2021

Conversation

WilliamKelley
Copy link
Contributor

@WilliamKelley WilliamKelley commented Jul 9, 2021

3 of 4

Part of #73. Last five commits are relevant. Reviewable by commit.

  1. Project generation (chore(nx): generate new project, spark-icons (@prenda/spark-icons) #132)
  2. Copying of icon utility scripts from Mui & Modification for our use case (chore(spark-icons): add icon file builder scripts #133)
  3. Removal of icons from @prenda/spark & Specific generation and replacement of depended-on icons (this PR)
  4. Addition of all icons in @prenda/spark-icons (feat(spark-icons): add all icons #135)

In this step

Removal and Replacement

All 1700 icon and index files were removed.

Only the icons we use in components/stories were generated and their usage replaced with deep imports -- we probably want to internalize to Spark whatever icons it uses, see #140 for discussion.

@WilliamKelley
Copy link
Contributor Author

WilliamKelley commented Jul 9, 2021

The deep imports are failing, looking into why

Example error msg:

ERROR in ./libs/spark/src/Select.ts
Module not found: Error: Can't resolve '@prenda/spark-icons/ChevronDown' in '/home/william/projects/spark/libs/spark/src'
 @ ./libs/spark/src/Select.ts 1:0-62 5:17-32

Fixed by modifying webpack.config.json, see last commit. Reading through nrwl/nx#2320 and https://webpack.js.org/guides/build-performance/#avoid-extra-optimization-steps helped me come to this fix.

@WilliamKelley WilliamKelley marked this pull request as ready for review July 14, 2021 15:34
Copy link
Contributor

@seigler seigler left a comment

Choose a reason for hiding this comment

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

This is really nice. I appreciate you wading into Webpack config to get the imports working!

@WilliamKelley WilliamKelley merged commit 32cf1ac into main Jul 15, 2021
@WilliamKelley WilliamKelley deleted the feat/Icons-removal branch July 15, 2021 17:44
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