Skip to content

[EuiIcon] Add generated file comment#5210

Merged
cee-chen merged 2 commits intoelastic:masterfrom
cee-chen:compile-icons-comment
Sep 22, 2021
Merged

[EuiIcon] Add generated file comment#5210
cee-chen merged 2 commits intoelastic:masterfrom
cee-chen:compile-icons-comment

Conversation

@cee-chen
Copy link
Contributor

Summary

Per @thompsongl's comments in #5204 (comment), we should add a comment to note that our icon .js files are generated files and shouldn't be modified (I had previously thought the .svg files were the ones that were for reference/archival and shouldn't be touched 🙃)

Checklist

N/A, internal/dev-only change

@cee-chen cee-chen added skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt labels Sep 22, 2021
@cee-chen cee-chen requested a review from thompsongl September 22, 2021 17:41
const iconFiles = glob.sync('**/*.svg', { cwd: iconsDir, realpath: true });

iconFiles.forEach(async filePath => {
iconFiles.forEach(async (filePath) => {
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 was a Prettier autofix on save

Comment on lines +58 to +59
const comment = '// THIS IS A GENERATED FILE. DO NOT MODIFY MANUALLY\n\n';
fs.writeFileSync(outputFilePath, comment + jsxSource);
Copy link
Contributor Author

@cee-chen cee-chen Sep 22, 2021

Choose a reason for hiding this comment

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

Comments in ast are a bit of a headache apparently (you have to pass in a custom preserveComments option, and then you have to use template.smart() instead of the template.ast shortcut, etc.) so I opted for the path of least resistance and just threw in the comment directly into the writeFile 🙃

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5210/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

just threw in the comment directly into the writeFile

I like it!

@cee-chen
Copy link
Contributor Author

🐇 Goin' down that rabbit hole: I'll look at .tsx for icons in a follow up PR!

@cee-chen cee-chen merged commit 565a0f2 into elastic:master Sep 22, 2021
@cee-chen cee-chen deleted the compile-icons-comment branch September 22, 2021 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants