-
Notifications
You must be signed in to change notification settings - Fork 8
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
Icons update #834
Icons update #834
Conversation
assets/package.json
Outdated
@@ -17,7 +17,8 @@ | |||
"lint": "eslint --ext .js,.jsx,.ts,.tsx ./js", | |||
"lint-fix": "eslint --ext .js,.jsx,.ts,.tsx ./js --fix", | |||
"test": "jest ./js/src --passWithNoTests", | |||
"storybook": "storybook dev -p 6006 --no-open" | |||
"storybook": "storybook dev -p 6006 --no-open", | |||
"generate-icons": "ts-node -r tsconfig-paths/register js/src/core/modules/icon-library/generate-icon-index.ts" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we call ts-node with npx ts-node
to avoid that it needs to be installed directly on the system?
I took a quick look at the options of ts-node and found two which should make it possible to overrule the compiler options from the defaut tsconfig.json:
-O, --compilerOptions [opts] JSON object to merge with compiler options
-P, --project [path] Path to TypeScript JSON project file
Maybe the --compilerOptions
is the simplest way to overrule the tsconfig and avoid that we need to change our default tsconfig.json
?
Also I'm not sure if the location inside of modules is the best one for the generate-icon-index.ts
. As it is a script related to the build maybe it's better to put it into assets/build
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did exactly this :) great suggestion
solved smaller icon inconsistencies and now detects when a svg file does not have a "stroke" in this case "fill" is being used |
cleaned up storybook. |
Quality Gate passedIssues Measures |
for the record. |
We have covered all the points on a call and most were resolved. On my side a couple of updates:
|
Changes in this pull request
Resolves #809
Additional info
To run the script go to studio-ui-bundle/assets and run:
npm run generate-icons
Find a quick "how to" at the top of the file:
I have updated the tsconfig.json and feel like this may have an impact.
Some naming changes should be made to the backend IconService.php
Details on Icons used / topics to discuss :
I have added icons / copied old ones and used icons that deviated to some extend from the old look to ensure all bases are covered.
This needs to be design reviewed.
Base Icons that were used:
https://www.figma.com/design/Q71sBiSRmHUDZhIqwEMVkm/Icon-Library?node-id=465-21&p=f&m=dev
reused what used to be the Icon "PlusCircleOutlined" under the name "new-circle"
used "file outlined" to replace the old "document" Icon
added the icons identically to the old icons:
I have noticed that the icon named "details" seems out of place now.
Below one can see how it has changed
there is a variant of data-object (one dark one light). Can we check which one should be used where? i.e. if everything is in the right place now.
the new icons also removed filled in circular warning, question mark etc. icons i.e.
there are two edit pens in the current icon library which we might want to revisit
I used "content" to replace the old "note"
The below Item is called "group" which does not fit what is on the svg
It can now be found in the list overview and we should revisit the naming.