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

Read and transform svg icons from list #3872

Merged
merged 6 commits into from
May 12, 2023
Merged

Read and transform svg icons from list #3872

merged 6 commits into from
May 12, 2023

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented May 11, 2023

2/4 main <= #3871 <= this <= #3873 <= #3874

Second step for #3848

This changes how SVGR works. Instead of using the CLI, there is now a script that takes all custom icons and specific codicons and generates the React components. This should make it easier to add and remove icons. I've also added some instructions in the contributing guide so that it's easy for anyone to add a new icon.

@sroy3 sroy3 self-assigned this May 11, 2023
@sroy3 sroy3 marked this pull request as ready for review May 11, 2023 19:51
@sroy3 sroy3 requested review from mattseddon and julieg18 as code owners May 11, 2023 19:51
This was referenced May 11, 2023
@codeclimate
Copy link

codeclimate bot commented May 11, 2023

Code Climate has analyzed commit 9131f25 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.9% (0.0% change).

View more on Code Climate.

@sroy3 sroy3 changed the base branch from main to icons-cleanup May 11, 2023 20:02
Copy link
Contributor

@julieg18 julieg18 left a comment

Choose a reason for hiding this comment

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

Great work! This will make handling icons a lot easier!

@@ -13,7 +13,7 @@
"test": "jest --collect-coverage",
"storybook": "storybook dev -p 6006",
"build-storybook": "storybook build --webpack-stats-json",
"svgr": "svgr --out-dir src/shared/components/icons --ignore-existing ../node_modules/@vscode/codicons/src/icons && svgr -d src/shared/components/icons icons/"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is my first time actually seeing this command! Looking at it, I'm assuming we used this to generate chosen icon components? I've just been creating them manually 🤣

Copy link
Contributor Author

@sroy3 sroy3 May 12, 2023

Choose a reason for hiding this comment

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

Yes, that is why I also added steps in the contributing guide. To be honest, I've been creating them manually as well for a while too because the process was broken. Running the command would generate every codicon, you would then have to remove all the ones you don't need.

webview/icons/generate.mjs Outdated Show resolved Hide resolved

You should also add any added icon to `webview/src/stories/Icons.stories.tsx`.

To use an icon, import it from `webview/src/shared/components/icons` (from the
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this last paragraph about importing? I feel like it would be obvious how to import and render an Icon by looking at the codebase (it definitely was for me when I needed an icon for the first time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends where you import it. If there are no example in the file you're trying to import the icon from, looking at our other imports, you might go directly for the file. I know I've made the comment more than once in PRs. It's mostly that it goes against how we import in general (using an index file).

webview/icons/generate.mjs Outdated Show resolved Hide resolved
@@ -38,6 +38,8 @@ jobs:

- run: yarn run install-frozen-lockfile

- run: yarn svgr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sure no one manually creates or edits icons.

* Automatically add new icons to the icons story in Storybook

* Add available codicons to Storybook (#3874)
@sroy3 sroy3 merged commit 46cba7e into icons-cleanup May 12, 2023
@sroy3 sroy3 deleted the svgr-list branch May 12, 2023 14:54
sroy3 added a commit that referenced this pull request May 12, 2023
* Cleaned up svg icons

* Read and transform svg icons from list (#3872)

* Read and transform svg icons from list

* Apply review comments

* Automatically add new icons to the icons story in Storybook (#3873)

* Automatically add new icons to the icons story in Storybook

* Add available codicons to Storybook (#3874)
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.

2 participants