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

Add shields for aur and make table of contents foldable #17

Merged
merged 5 commits into from May 10, 2023

Conversation

neon-mmd
Copy link
Contributor

@neon-mmd neon-mmd commented May 9, 2023

I have added two shields to show the status of aur package and for the maintainance status and also made table of contents foldable, this will make the readme feel less lengthy for the reader.

Copy link
Owner

@maxcurzi maxcurzi left a comment

Choose a reason for hiding this comment

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

I like the collapsible TOC, but the badges are too many and it feels too cluttered.

Leave only the AUR 4.xxx badge but please take the others out.

@neon-mmd
Copy link
Contributor Author

Ok sure no problem, I will take it out 😄 .

@neon-mmd
Copy link
Contributor Author

Ok as per your request I removed it 😄 .

@neon-mmd neon-mmd requested a review from maxcurzi May 10, 2023 07:53
@maxcurzi
Copy link
Owner

I meant remove all new badges except the one that points to the AUR package (the one with the version number) ☺️

@neon-mmd
Copy link
Contributor Author

neon-mmd commented May 10, 2023

I meant remove all new badges except the one that points to the AUR package (the one with the version number) relaxed

My bad, I took it in the other way. just spare me a few minutes and I will come back to you.

One more thing before I make the change, I think the maintainance status shield would be necessary because this will show the reader that yes your project is well maintained and active and it is not a dead project 😄 . If you wish I can make the badges and the project title look something similar to this one:

image

or like this one:

image

@maxcurzi
Copy link
Owner

I meant remove all new badges except the one that points to the AUR package (the one with the version number) relaxed

My bad, I took it in the other way. just spare me a few minutes and I will come back to you.

One more thing before I make the change, I think the maintainance status shield would be necessary because this will show the reader that yes your project is well maintained and active and it is not a dead project smile . If you wish I can make the badges and the project title look something similar to this one:

image

or like this one:

image

I understand but I still don't like it.

It doesn't take much to see if a project is maintained or not, in fact I think it's a better practice to understand what's the maintenance status of a project based on commits, PR's etc... and form your own opinion/judgement.

@neon-mmd
Copy link
Contributor Author

Agreed 👍, Ok so in few minutes I am going to make the change.

@neon-mmd
Copy link
Contributor Author

Ok done 👍 , you review the change again 😄 .

README.md Outdated
Comment on lines 11 to 18

<a href=""
><img
alt="Maintenance"
src="https://img.shields.io/maintenance/yes/2023?style=flat-square"
/>
</a>

Copy link
Owner

Choose a reason for hiding this comment

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

remove this!

Copy link
Contributor Author

@neon-mmd neon-mmd May 10, 2023

Choose a reason for hiding this comment

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

I made this change here in this commit 716a797 😄 .

Copy link
Owner

@maxcurzi maxcurzi May 10, 2023

Choose a reason for hiding this comment

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

Sure, but you should remove this code, too.

Otherwise there is still a "Maintenance" badge which – aside from the fact that it has a different visual style – I don't think it's necessary.

image

@@ -7,31 +7,45 @@
[![Crates.io](https://img.shields.io/crates/v/tplay)](https://crates.io/crates/tplay)
[![Crates.io](https://img.shields.io/crates/d/tplay)](https://crates.io/crates/tplay)
[![License](https://img.shields.io/badge/license-MIT-blue)](LICENSE)
[![Aur](https://img.shields.io/aur/version/tplay-git)](https://aur.archlinux.org/packages/tplay-git)
Copy link
Owner

Choose a reason for hiding this comment

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

this is ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can merge this now 😄 .

Copy link
Owner

@maxcurzi maxcurzi left a comment

Choose a reason for hiding this comment

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

see comments

@neon-mmd
Copy link
Contributor Author

neon-mmd commented May 10, 2023

Also a few areas to improve in your project:

  • invest some time in writing a good code of conduct
  • write a pull request template
  • a workflows file. it is very easy to write and it is not that difficult you can watch this tutorial on how to do it.
  • Also for discussions related to the project provide a link to discord, matrix, irc or something where people can ask, learn or know about the project.

@neon-mmd neon-mmd reopened this May 10, 2023
@neon-mmd
Copy link
Contributor Author

neon-mmd commented May 10, 2023

Ok finally I realized my mistake 😄 that I had mistakenly added a maintanance status badge with html but I have now resolved the error you can review the change again.

Note: I closed and reopened the issue because I thought what's wrong why is it not updating but finally realized my mistake as mentioned above and reopened this issue :).

@neon-mmd neon-mmd requested a review from maxcurzi May 10, 2023 13:26
@maxcurzi maxcurzi merged commit d8288ee into maxcurzi:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants