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 a GitHub discussions badge to the README #1299

Merged
merged 1 commit into from
Jun 10, 2024

Conversation

mjang
Copy link
Collaborator

@mjang mjang commented Jun 3, 2024

I need to update this with the NGINX color code.

I don't claim to know any/all PR requirements -- I'm using this as a learning experience.

ref nginx/unit-docs#150

README.md Outdated
@@ -2,6 +2,7 @@

[![Project Status: Active – The project has reached a stable, usable state and is being actively developed.](https://www.repostatus.org/badges/latest/active.svg)](https://www.repostatus.org/#active)
[![CI](https://github.com/nginx/unit/actions/workflows/ci.yml/badge.svg)](https://github.com/nginx/unit/actions/workflows/ci.yml "GitHub workflow CI")
[![Discussions](https://img.shields.io/badge/N_Unit-discussions-green)](https://github.com/nginx/unit/discussions "NGINX Unit Discussion Board")
Copy link
Member

Choose a reason for hiding this comment

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

Please make the alt text GitHub Discussions

I think it would be better if the badge text read NGINX Unit rather than 'N Unit'.

Also the 'green' seems a little off, interestingly if you chop off the 'n' (and more) from green you get a better colour, e.g

https://img.shields.io/badge/N_Unit-discussions-gree

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for finding the "closer" color. Not sure why, but it seems equivalent to https://img.shields.io/badge/N_Unit-discussions-g

The suggested alt text makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, actually I'm wondering if GitHub | discussions would make more sense than NGINX Unit | discussions?

What do other projects use?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ac000 , from a UI Text PoV, we'll see this badge only in the README for this well-labeled NGINX repo. So I think it's OK to label this GitHub | discussions.

Copy link
Member

@ac000 ac000 left a comment

Choose a reason for hiding this comment

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

I think the commit subject would be better as

Add a GitHub discussions badge to the README

@mjang mjang force-pushed the mjang-README-update-discussions branch from 1465441 to ea5e0a6 Compare June 4, 2024 16:33
@mjang mjang changed the title Include link to discussions page Add a GitHub discussions badge to the README Jun 4, 2024
@mjang
Copy link
Collaborator Author

mjang commented Jun 4, 2024

I think the commit subject would be better as

Add a GitHub discussions badge to the README

I agree and have updated the Git commit

README.md Outdated
@@ -2,6 +2,7 @@

[![Project Status: Active – The project has reached a stable, usable state and is being actively developed.](https://www.repostatus.org/badges/latest/active.svg)](https://www.repostatus.org/#active)
[![CI](https://github.com/nginx/unit/actions/workflows/ci.yml/badge.svg)](https://github.com/nginx/unit/actions/workflows/ci.yml "GitHub workflow CI")
[![GitHub Discussions](https://img.shields.io/badge/NGNIX_Unit-discussions-g)](https://github.com/nginx/unit/discussions "GitHub Discussions")
Copy link
Member

Choose a reason for hiding this comment

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

How about we use

https://img.shields.io/badge/GitHub-Discussions-009639

For the badge URL?

(That's NGINX green, well the green from the N logo from the README anyway...)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You have a point. I've updated the color for all three badges, and they seem to more closely match the color of the NGINX logo:

Screenshot 2024-06-05 at 1 58 11 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OTOH, the text in the green part of the badges are a little more difficult to read? (#feEngineeringIsHarderThanItLooks)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For comparison, here's the "current" green in main

Screenshot 2024-06-05 at 2 02 00 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ac000 , I'll go with whatever you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

Ooh I like that!

The main problem though is that the 'CI' badge changes dynamically depending on whether the workflows pass or not.

According to it may may be doable using https://shields.io/badges/endpoint-badge

But I'd want this as two separate patches anyway, so do the first patch that just adds the new badge, using GitHub instead of NGINX Unit for the left-hand side of the badge.

Then we can look at re-colouring the others...

Copy link
Collaborator Author

@mjang mjang Jun 7, 2024

Choose a reason for hiding this comment

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

The main problem though is that the 'CI' badge changes dynamically depending on whether the workflows pass or not.

Hi @ac000, I didn't know this.

So I'll limit this PR to adding the GitHub Discussions badge. Since we want to "re-color" ( ;) ) the others, I'll set up a follow-up issue, #1306.

@mjang mjang force-pushed the mjang-README-update-discussions branch from ea5e0a6 to 5f1261e Compare June 5, 2024 20:56
@ac000
Copy link
Member

ac000 commented Jun 6, 2024

Actually, while you're at it, you could create another separate patch that adds a 'tooltip' for the repo status button, we have one for the CI button, something like "Repo Status - Active"

@ac000
Copy link
Member

ac000 commented Jun 7, 2024

Hi @mjang

Looks good. Just one thing, as a general rule we use our corporate email addresses (@f5.com/@nginx.com) when committing.

You can set it in the unit repository via

$ cd /path/to/unit
$ git config user.email EMAIL-ADDRESS

You can also adjust your commit to use it with

$ git commit --amend --no-edit --author="NAME <EMAIL-ADDRESS>"

@mjang mjang force-pushed the mjang-README-update-discussions branch from 7b389e0 to 5cae808 Compare June 10, 2024 14:51
@mjang
Copy link
Collaborator Author

mjang commented Jun 10, 2024

Hi @mjang

Looks good. Just one thing, as a general rule we use our corporate email addresses (@f5.com/@nginx.com) when committing.

Hi @ac000 . Thanks for the guidance. I've now updated the commit with my corporate email address. This should now be ready for approval.

- With NGINX green (hex code 009639)

Signed-off-by: Andrew Clayton <[email protected]>
@mjang mjang self-assigned this Jun 10, 2024
@ac000 ac000 force-pushed the mjang-README-update-discussions branch from 5cae808 to 98983f3 Compare June 10, 2024 16:00
@ac000
Copy link
Member

ac000 commented Jun 10, 2024

  • Rebased with master
  • Added my s-o-b
$ git range-diff 5cae8086...98983f3f
-:  -------- > 1:  e77a0c16 Tests: explicitly specify 'f' prefix to format string before printing
-:  -------- > 2:  c9dced37 Tests: print unit.log on unsuccessful unmount
1:  5cae8086 ! 3:  98983f3f Add a GitHub discussions badge to the README
    @@ Commit message
     
         - With NGINX green (hex code 009639)
     
    +    Signed-off-by: Andrew Clayton <[email protected]>
    +
      ## README.md ##
     @@
      

@ac000 ac000 merged commit 98983f3 into nginx:master Jun 10, 2024
18 of 19 checks passed
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.

2 participants