-
Notifications
You must be signed in to change notification settings - Fork 29
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
Added primary changes to add page for Removed icons #159
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.
.DS_Store
should not be committed.
Just a small tip - you should add it to your global gitignore |
@@ -0,0 +1,314 @@ | |||
module.exports = [ |
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.
We need a way to update this file automatically. Do you have something in mind about it @jorgeamadosoria?
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.
No. I don't really mind updating this manually. We don't remove that many icons, this is easy to keep updated just by reading the Release notifications and getting the removed issues from there.
I'm fine with this setup.
Of course, if you can think of a way to automate it, I'm all ears. The less work, the better.
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 think that it should be inside the "Auto Release" workflow, after the step that bumps simple-icons version.
Technically would be "if there is a major version change, compare the icons in latest release with the new ones, and add those deleted to the file".
No. I don't really mind updating this manually.
If so, you maybe need to create a release checklist and declare some compromise to maintain it. Anyways, please, note that we don't want to depend on a single contributor for periodical manual tasks. If you left the organization tomorrow for whatever reason someone else would be obliged to keep this, so I am totally against not automating it.
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.
yeah, no I get that. Not sure how to add this to the Auto Release workflow. I'll take a look.
doctype html | ||
html(lang='en-US') | ||
head | ||
meta(charset='UTF-8') | ||
meta( | ||
content='initial-scale=1, shrink-to-fit=no, width=device-width', | ||
name='viewport' | ||
) | ||
link(rel='preconnect', href='https://fonts.gstatic.com') | ||
title #{ pageTitle } | ||
meta(name='description', content=pageDescription) | ||
meta(property='og:type', content='website') | ||
meta(property='og:title', content=pageTitle) | ||
meta(property='og:description', content=pageDescription) | ||
meta(property='og:url', content=pageUrl) | ||
meta(property='og:site_name', content=pageTitle) | ||
meta(property='og:image', content=`${pageUrl}/images/og.png`) | ||
link(rel='icon', type='image/x-icon', href=`${pageUrl}/images/favicon.ico`) | ||
link(rel='icon', type='image/png', href=`${pageUrl}/images/favicon.png`) | ||
link(rel='apple-touch-icon', href=`${pageUrl}/images/apple-touch-icon.png`) | ||
link(rel='mask-icon', href=`${pageUrl}/images/logo.svg`, color='#111111') | ||
link( | ||
rel='stylesheet', | ||
href='https://fonts.googleapis.com/css2?family=Open+Sans:wght@600&family=Roboto+Mono:wght@400;600&display=swap' | ||
) | ||
link(rel='license', href='license.txt') | ||
link(rel='canonical', href=pageUrl) |
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.
The head and the footer should be reused between the two pages. See Template Inheritance with Pug.
new HtmlWebpackPlugin({ | ||
inject: true, | ||
filename: 'removed.html', | ||
template: path.resolve(ROOT_DIR, 'removed.pug'), | ||
templateParameters: { | ||
removedIcons: removedIcons | ||
.sort((v1, v2) => v1.title.localeCompare(v2.title)) | ||
.map((icon, iconIndex) => { | ||
const luminance = getRelativeLuminance(`#${icon.hex}`); | ||
return { | ||
base64Svg: Buffer.from(icon.path).toString('base64'), | ||
hex: icon.hex, | ||
light: luminance < 0.4, | ||
superLight: luminance > 0.95, | ||
superDark: luminance < 0.02, | ||
issue: icon.issue, | ||
versionIssue: icon.versionIssue, | ||
path: icon.path, | ||
title: icon.title, | ||
}; | ||
}), | ||
iconCount: removedIcons.length, | ||
pageTitle: 'Removed Icons', | ||
pageDescription: | ||
"Removed icons in all previous versions. Some icons can't be shown since they were removed because the brand does not allow them to be modified to fit this library.", | ||
pageUrl: 'https://simpleicons.org/removed', |
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.
A lot of configuration can also be reused between the two pages, this should help us maintaining the project.
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.
agreed.
The changes necessary to automate the removed icons page are not worth the trouble. I'm closing this in favor of greater focus on the other open PRs on the website. |
Added Removed icons page, to contain all deleted icons in previous version with a format similar to the one at the main page. Some icons that were removed because the brand does not allow modifications are therefore listed, but not shown in compliance with the reason they were removed (i.e., Fedora, Yandex, etc).
Pending changes: navigation to and fro the main page.
Alternatively, we can just forgo navigation and link this from the Readme in the main repository, as a note.