Skip to content
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

Update star icons in classic template #186

Merged
merged 15 commits into from
Mar 16, 2021
Merged

Update star icons in classic template #186

merged 15 commits into from
Mar 16, 2021

Conversation

Milo123459
Copy link
Contributor

  • filled star system
  • activity: filled star system

Now, when you star a repository, it'll use a filled star icon rather then a blank one (to make it look more like the original GitHub)

@Milo123459
Copy link
Contributor Author

This also removes libxmljs, I couldn't see it doing anything major and I've started looking into a better soloution rather then using libxmljs.

Copy link
Owner

@lowlighter lowlighter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify feature should not be removed as it is used for units test.

Pupeeteer postinstall and template edits looks nice 👍🏼

package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Show resolved Hide resolved
source/app/metrics/index.mjs Outdated Show resolved Hide resolved
@Milo123459
Copy link
Contributor Author

So this isn't going to be merged until we find an alternative for libxmljs?

@Milo123459
Copy link
Contributor Author

Preview:
img1
img2
img3
img4

@lowlighter
Copy link
Owner

So this isn't going to be merged until we find an alternative for libxmljs?

Unless you rebase this to only keep templates changes (though dev and postinstall scripts are fine, though it would be better in another pr too)

It's better to make separate pull requests when scopes are differents, it's less confusing and help to keep track of changes.
Ideally, a pull request content should only have a single subject (e.g. a new feature, same-kind fixes, etc).
It's weird that a pr suggesting patching templates impacts dependencies, core code along with repo-releated files 😅


A few additional notes which prevents this to be merged:

  • modern-normalize should be removed since it doesn't work here
  • verify option code should be restored (required by tests), along with libxml (or libxml moved to optionalDependencies)
  • linter:fix seems useless, you can already pass arguments to npm scripts using --, like npm run linter -- --fix
  • some svg icons are badly indented (it's minor, but since ejs files are already hard to read, it would be nice to keep indent consistency)

Hope it doesn't discourage you 🙂

@Milo123459
Copy link
Contributor Author

So this isn't going to be merged until we find an alternative for libxmljs?

Unless you rebase this to only keep templates changes (though dev and postinstall scripts are fine, though it would be better in another pr too)

It's better to make separate pull requests when scopes are differents, it's less confusing and help to keep track of changes.
Ideally, a pull request content should only have a single subject (e.g. a new feature, same-kind fixes, etc).
It's weird that a pr suggesting patching templates impacts dependencies, core code along with repo-releated files 😅

A few additional notes which prevents this to be merged:

  • modern-normalize should be removed since it doesn't work here
  • verify option code should be restored (required by tests), along with libxml (or libxml moved to optionalDependencies)
  • linter:fix seems useless, you can already pass arguments to npm scripts using --, like npm run linter -- --fix
  • some svg icons are badly indented (it's minor, but since ejs files are already hard to read, it would be nice to keep indent consistency)

Hope it doesn't discourage you 🙂

Verify is back, linter:fix is indeed useless, some svg are badly indented, modern-normalize will be removed and other things should be fixed soon.

@Milo123459
Copy link
Contributor Author

I still can't install libxmljs. I have python2.7 installed, ran it with elevated permissions, and nothing. Still doesn't work. I fixed everything else so I will commit that but then this just isn't working!

@Milo123459
Copy link
Contributor Author

e

There is our smoking gun

@Milo123459
Copy link
Contributor Author

I just forcefully wrote it into the package.json.

@lowlighter lowlighter changed the title filled star Update star icons in classic template Mar 16, 2021
@lowlighter lowlighter merged commit 9754817 into lowlighter:master Mar 16, 2021
@lowlighter
Copy link
Owner

I'm not sure what to think about the change of stars/sponsors icons actually.

It's kind of fine on dark mode but it really look odd on light mode.
I think that's because all of others icons are just strokes and using filled icons looks out of place:

@Milo123459
Copy link
Contributor Author

Hmm..

@github-actions github-actions bot locked and limited conversation to collaborators Apr 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants