-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Major build improvements #1668
Major build improvements #1668
Conversation
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.
I haven't reviewed everything yet, but I don't have more time today, so I figured I'd post this in case you want to look at it before I get the chance to review the rest :)
Co-authored-by: Jørgen Kalsnes Hagen <[email protected]>
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.
Looks good to me! 🚀
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.
README looks good now 💯
One of the nano icons is still changed and needs to be reverted though.
Other than that I mostly just have some questions to better understand the code.
Not much left now until we can merge 🚀
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.
This change still needs to be reverted
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.
I reverted all the changes in the icon; not sure what else should I do. 🤔
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.
Odd, maybe we can try and find which commit the file was changed in?
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.
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.
All I did is copy the nano
icon contents from the develop
branch. Probably some Git-meta changes or such.
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.
Ahhh, have you tried git revert
?
</head> | ||
|
||
<body ng-app="devicon" ng-controller="IconListCtrl"> | ||
<link rel="stylesheet" ng-if="latestReleaseTagging" ng-href="https://cdn.jsdelivr.net/gh/devicons/devicon@{{ latestReleaseTagging }}/devicon.min.css"> |
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.
Is this not needed anymore? Could you explain what it did, and why we don't need it anymore?
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.
- Yes it is needed, but it is added by the script above.
- The stylesheet file (
devicon.min.css
) loads all the icons data, colors, etc.. - As I said, it is added by the JavaScript script above.
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.
I see. It would probably be less code to move the script to the .js file and add a variable, then use that variable here instead of adding the element via the DOM.
That would more closely follow the SOLID principle too
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.
So, you're saying to move the whole script to another JS file and load the script via HTML?
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.
Yes, sort of. Moving it to the .js file, but using Angular features instead of interacting directly with the DOM. It's less complex and less code that way.
Co-authored-by: Jørgen Kalsnes Hagen <[email protected]>
Since this branch has some permissions, here goes the updated fonts. update-fonts.zip 📚
This
|
@lunatic-fox care to share the script? :) |
I made it first with local files, but I've made another version using fetch. const fs = require('fs');
const baseURL = 'https://cdn.jsdelivr.net/gh/devicons/devicon@develop/';
(async() => {
const devicon = await(await fetch(`${baseURL}devicon.json`)).json();
for (const e of devicon) {
for (const f of e.versions.font) {
const icon = await fetch(`${baseURL}icons/${e.name}/${e.name}-${f}.svg`);
if (icon.status === 200) {
console.log(`> ${e.name}-${f}`);
fs.writeFileSync(`icomoon/${e.name}-${f}.svg`, await (icon.text()));
}
}
}
})(); |
Here goes a faster version: const fs = require('fs');
const baseURL = 'https://cdn.jsdelivr.net/gh/devicons/devicon@develop/';
(async() => {
let x = 0, y = 10;
const devicon = await(await fetch(`${baseURL}devicon.json`)).json();
const toBuild = [];
while (true) {
const deviconChunk = devicon.map(e => ({
name: e.name,
font: e.versions.font
})).slice(x, y);
if (deviconChunk.length === 0) break;
for (const e of deviconChunk) {
for (const f of e.font) {
const icon = await fetch(`${baseURL}icons/${e.name}/${e.name}-${f}.svg`);
if (icon.status === 200) {
const svg = await icon.text();
toBuild.push({
path: `${e.name}-${f}.svg`,
icon: svg
});
console.log(`> ${e.name}-${f}`);
}
}
}
x += 10, y += 10;
}
toBuild.forEach(e => fs.writeFileSync(`icomoon/${e.path}`, e.icon));
})(); |
* Make README more readable * move logic from index.html to script.js * fix devicon min css in head * Fix build script missing token error * Add more print statements to local build script * Change from ng-href to href in code view * Minor readme improvements --------- Co-authored-by: David Leal <[email protected]>
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.
Looks good! ✔️
Like we already discussed, I had trouble running it locally. If the issue persists we can look at that later. For now it's good enough that it runs on GitPod imo :)
Let's get this release out ASAP :D
Things added/changed:
font
folder changes ordevicon-base.css
) weren't changed. Not sure why GitHub displays it like that. 🤔NOTE: This will need proper testing. I already tested and it works perfectly fine without any issues.
Any doubts, suggestions, or questions, please let me know about it. Thanks! 🙂