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

Add optional input and output directory arguments to scripts/generate-icons.js #1064

Merged
merged 2 commits into from
May 30, 2023

Conversation

robertknight
Copy link
Member

@robertknight robertknight commented May 30, 2023

Add optional arguments to scripts/generate-icons.js to allow custom input/output dirs to be specified. This allows the script to be re-used to generate icons in other projects, or for other purposes. Per commit notes, I'm not using a proper CLI parser because the args are so simple. This can be changed if the syntax gets more complex.

The initial use case is to generate icon components for the video player app. The changes in this PR are enough for that. It would be convenient if the script was included in the package, then we could add a package.json script to run it in the downstream project.

While testing this I noticed that incremental runs of ESLint and Prettier were taking a long time because caching was not enabled, so I fixed that in a separate commit.

Enable generate-icons.js to be used from other projects where the input and
output directory paths may be different.

I didn't add a CLI parser dependency because the arguments are so simple, but
we should add one if they become more complex.
This speeds up incremental runs of `yarn format` and `yarn lint` significantly.
@acelaya
Copy link
Contributor

acelaya commented May 30, 2023

Should we perhaps add all icons used by our apps in frontend-shared instead, and then use the already generated icons there?

That would mean we don't need to modify the script, but I might be missing something.

@robertknight
Copy link
Member Author

Should we perhaps add all icons used by our apps in frontend-shared instead, and then use the already generated icons there?

If icons are generally useful, it makes sense to include them in this package. However I do think it would be useful to have a way to build and iterate on project-specific app icons without having to always add them to the shared package. For example if we have SVG icons for the different content sources in the LMS app, I think it makes sense to keep them in that project alongside the code that uses them.

@acelaya
Copy link
Contributor

acelaya commented May 30, 2023

Good point.

It will also allow for faster prototyping with the option to move the icons here later, if it makes sense.

Copy link
Contributor

@acelaya acelaya left a comment

Choose a reason for hiding this comment

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

👍🏼

@robertknight robertknight merged commit 0a10b93 into main May 30, 2023
@robertknight robertknight deleted the generate-icons-reuse branch May 30, 2023 14:13
@lyzadanger
Copy link
Contributor

Just wanted to add a comment: I agree that typically we want to include icon components in this package, so that they can be seen as a collection in the pattern library and are generally organized in one place, but I think it's reasonable to generate icon components in other projects from time to time, especially if you're prototyping or otherwise unsure that the icon in question will have broader use.

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