Feat | crayons-icon CLI Tooling #350
Conversation
| * @freshworks/crayons-icon | ||
| * Crayons-Icon as NodeJS-CLI Tool to optimize SVGs and generate JS Imports for icons. | ||
| * | ||
| * @author Freshworks <https://freshworks.in> |
There was a problem hiding this comment.
freshworks.in is not the right domain :)
packages/crayons-icon/CHANGELOG.md
Outdated
| @@ -2,9 +2,3 @@ | |||
|
|
|||
| All notable changes to this project will be documented in this file. | |||
There was a problem hiding this comment.
Why is the changeLog removed ?
packages/crayons-icon/package.json
Outdated
| "svgo yml config", | ||
| "svg JS Exports", | ||
| "svg optimizer", | ||
| "crayons-icon-cli" |
There was a problem hiding this comment.
can u check the formatting here ?
packages/crayons-icon/readme.md
Outdated
| function App() { | ||
| return ( | ||
| <div> | ||
| < FwIcon dataSvg = { header } label = "Header"/> |
There was a problem hiding this comment.
Pls remove the space after the starting tag token. It should be <FwIcon
| "devDependencies": { | ||
| "fs": "^0.0.1-security", |
There was a problem hiding this comment.
Why is fs added as a dev dependency ?
| } | ||
| async function optimizeSvg(optimizePass, svgData) { | ||
| const srcSvgContent = await fs_extra.readFile(svgData.srcFilePath, 'utf8'); | ||
| const optimizedSvg = await optimizePass.optimize(srcSvgContent, { |
There was a problem hiding this comment.
Please handle exceptions in the application.
There was a problem hiding this comment.
Centralized level in build block, Tested. I cannot add try-catch{} in all functions as it will result in eslint error: 'Unnecessary try-catch wrapper '. So done at the central level.(fn)build.
Fixed
| const distSvgFilePath = path.join(distSvgDir, fileName); | ||
| const dotSplit = fileName.split('.'); | ||
| if (dotSplit.length > 2) { | ||
| throw new Error( |
There was a problem hiding this comment.
How is the error handled in this application ?
There was a problem hiding this comment.
Centralized level in build block, Tested. I cannot add try-catch{} in all functions as it will result in eslint error: 'Unnecessary try-catch wrapper '. So done at the central level.(fn)build.
There was a problem hiding this comment.
@detiwari003 I would recommend that from next time you don't club a lot of changes into one PR. it's easy to code and review one type of change you are making pushing it out!
Yes Parsu. Will do the same. I have just refactored the earlier implementation with proper variable and function names and retested exception cases, added try catch where necessary and removed unnecessary exception stack logging by using e.message.
|
|
||
| @Element() el: HTMLElement; | ||
| @State() private setElVisible = false; |
There was a problem hiding this comment.
Why is this a State? can't it just be a private variable?
There was a problem hiding this comment.
@parsuram-vijaysankar The @State() decorator is used to manage internal data for a component. This means that a user cannot modify this data from outside the component, but the component can modify it however it sees fit. Any changes to a @State() property will cause the components render function to be called again.
This is what we need in xOb.
Checklist:
How Has This Been Tested?