-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
refactor icons.json #2084
refactor icons.json #2084
Conversation
icons.json name=>displayName, className=>iconNamePreview: documentation | landing | table |
function toShortName(icon) { | ||
return icon.className.replace("pt-icon-", ""); | ||
/** | ||
* Removes `pt-icon-` prefix from icon className. |
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.
that's not what this does
@@ -111,13 +112,13 @@ function exportIconConsts(valueGetter) { | |||
async function buildPathsObject(objectName, size) { | |||
return Promise.all( | |||
ICONS_METADATA.map(async icon => { | |||
const filepath = path.resolve(__dirname, `../../resources/icons/${size}px/${icon.className}.svg`); | |||
const filepath = path.resolve(__dirname, `../../resources/icons/${size}px/${toLongName(icon)}.svg`); |
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.
can we batch rename the SVG files too? no need for pt- in the filename
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.
oof, sure
also chmod -x all these files (why were they executable in the first place??)
|
||
// map ENUM_NAME to pt-icon-class-name | ||
writeLinesToFile("iconClasses.ts", ...exportIconConsts(icon => icon.className)); | ||
writeLinesToFile("iconClasses.ts", ...exportIconConsts(toLongName)); |
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.
@adidahiya thoughts on deleting this file as it's simply Classes.iconClass(IconNames.AIRPLANE)
?
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.
hm yeah I guess that's fine... can you add it to the migration script?
docsPreview: documentation | landing | table |
are you going to delete |
separate PR
…On Tue, Feb 6, 2018 at 12:45 PM Adi Dahiya ***@***.***> wrote:
are you going to delete iconClasses.ts?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#2084 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAcXttijKVXMo_KyZ2VdGPCAie6wlNR2ks5tSLnlgaJpZM4R6I-j>
.
|
Changes proposed in this pull request:
icons.json
name=>displayName, className=>iconNameclassName
withpt-icon-
prefix is a bunch of crufty data, esp given iconName => icon prop #2070generate-icons-source
Reviewers should focus on:
this PR should fix the broken Icons page in #2070
new record for most files changed?
870