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

fix badge style when logo only #10794

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

LitoMore
Copy link
Contributor

@LitoMore LitoMore commented Jan 9, 2025

Resolves #10628

It looks like there is no unit test for this. The badges below can used for previewing:

![](https://img.shields.io/badge/-blue?logo=amg)
![](https://img.shields.io/badge/-blue?logo=amg&style=social)
![](https://img.shields.io/badge/-blue?logo=amg&style=plastic)
![](https://img.shields.io/badge/-blue?logo=amg&style=flat-square)
![](https://img.shields.io/badge/-blue?logo=amg&style=for-the-badge)

![](https://img.shields.io/badge/-blue?logo=amg&logoSize=auto)
![](https://img.shields.io/badge/-blue?logo=amg&logoSize=auto&style=social)
![](https://img.shields.io/badge/-blue?logo=amg&logoSize=auto&style=plastic)
![](https://img.shields.io/badge/-blue?logo=amg&logoSize=auto&style=flat-square)
![](https://img.shields.io/badge/-blue?logo=amg&logoSize=auto&style=for-the-badge)









Copy link
Contributor

github-actions bot commented Jan 9, 2025

Messages
📖 ✨ Thanks for your contribution to Shields, @LitoMore!

Generated by 🚫 dangerJS against e05753f

@chris48s chris48s added the npm-package Badge generation and badge templates label Jan 10, 2025
Copy link
Contributor

🚀 Updated review app: https://pr-10794-badges-shields.fly.dev

@chris48s
Copy link
Member

Can we add snapshot test cases for this

@LitoMore
Copy link
Contributor Author

@chris48s Where should I add the snapshot test? The badge-maker/lib/make-badge.spec.mjs looks not in the test:package's scope. The test:package only runs .js files.

@chris48s
Copy link
Member

Oh crap. Good catch. Thanks!
I accidentally broke this in #10732 😞

I've submitted a PR at #10809 fixing this
Lets get that PR in, then we can come back to this ad add tests.

@chris48s
Copy link
Member

You should be able to finish off the tests on this one now

@LitoMore LitoMore force-pushed the fix-badge-style-when-logo-only branch from 1a70f04 to 2a29628 Compare January 21, 2025 02:35
@LitoMore LitoMore requested a review from chris48s January 21, 2025 02:35
Copy link
Contributor

🚀 Updated review app: https://pr-10794-badges-shields.fly.dev

@chris48s
Copy link
Member

There's a bug here in the case where there is a message and logo but no label:

example call: https://img.shields.io/badge/just%20the%20message-blue?logo=javascript

master: before
this branch: after

Note how the end of the message is cut off in the second image.

I think those changes where you're updating the snapshot width

-  width="63"
+  width="49"

are changing because of this bug.

@LitoMore
Copy link
Contributor Author

@chris48s It should be fixed now.

Copy link
Contributor

🚀 Updated review app: https://pr-10794-badges-shields.fly.dev

@chris48s
Copy link
Member

OK, cool. So that's that bug fixed.

So now I'm just having a look over the test cases and the snapshots generated from your tests in a bit more detail.

There's a few things here..

@chris48s
Copy link
Member

If we zoom back out to the original point of this issue, the thing you're actually trying to change is the case where both label and message are empty string, and that's the thing we'd like to get under test. But that is the one thing you don't seem to have added any test cases for in this PR. Every test case added in this PR has some text in either the label or message field.

We already have existing tests in this file with badges with a logo for each of the 5 styles with text in the label or message so a lot of these are trying to duplicate existing test cases.

I think the only new test cases we actually need to add here to cover the changes you're trying to make are:

{
  label: '',
  message: '',
  format: 'svg',
  logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
  style: 'flat',
}

{
  label: '',
  message: '',
  format: 'svg',
  logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
  style: 'flat-square',
}

{
  label: '',
  message: '',
  format: 'svg',
  logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
  style: 'for-the-badge',
}

{
  label: '',
  message: '',
  format: 'svg',
  logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
  style: 'social',
}

{
  label: '',
  message: '',
  format: 'svg',
  logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
  style: 'plastic',
}

@LitoMore
Copy link
Contributor Author

@chris48s Updated, thanks for reviewing!

@LitoMore LitoMore requested a review from chris48s January 24, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm-package Badge generation and badge templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants