-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
base: master
Are you sure you want to change the base?
Conversation
🚀 Updated review app: https://pr-10794-badges-shields.fly.dev |
Can we add snapshot test cases for this |
@chris48s Where should I add the snapshot test? The |
You should be able to finish off the tests on this one now |
1a70f04
to
2a29628
Compare
🚀 Updated review app: https://pr-10794-badges-shields.fly.dev |
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 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. |
@chris48s It should be fixed now. |
🚀 Updated review app: https://pr-10794-badges-shields.fly.dev |
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.. |
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:
|
@chris48s Updated, thanks for reviewing! |
it('social badge, logo-only', async function () { | ||
await expectBadgeToMatchSnapshot({ | ||
label: '', | ||
message: '', | ||
format: 'svg', | ||
logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu', | ||
style: 'for-the-badge', | ||
}) | ||
}) | ||
|
||
it('plastic badge, logo-only', async function () { | ||
await expectBadgeToMatchSnapshot({ | ||
label: '', | ||
message: '', | ||
format: 'svg', | ||
logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu', | ||
style: 'for-the-badge', |
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 style
values in the test data and the descriptions of the tests here don't match up.
Rather than go through another round of back and forth on this I'm just going to push another commit fixing it so we can get done with this one.
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.
OK scratch that. I don't have permission to push to your branch.
Here's a patch to apply:
commit 61c5f021d565f7161c217eb9d20a2166083627e7
Author: chris48s <git@chris-shaw.dev>
Date: Fri Jan 24 21:13:42 2025 +0000
align test cases and descriptions
diff --git a/__snapshots__/make-badge.spec.mjs.js b/__snapshots__/make-badge.spec.mjs.js
index 4c0277862a..861e70e727 100644
--- a/__snapshots__/make-badge.spec.mjs.js
+++ b/__snapshots__/make-badge.spec.mjs.js
@@ -2624,7 +2624,7 @@ exports['The badge generator badges with logo-only should always produce the sam
`
-exports['The badge generator badges with logo-only should always produce the same badge social badge, logo-only 1'] = `
+exports['The badge generator badges with logo-only should always produce the same badge for-the-badge badge, logo-only 1'] = `
<svg
xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink"
@@ -2664,41 +2664,121 @@ exports['The badge generator badges with logo-only should always produce the sam
`
+exports['The badge generator badges with logo-only should always produce the same badge social badge, logo-only 1'] = `
+<svg
+ xmlns="http://www.w3.org/2000/svg"
+ xmlns:xlink="http://www.w3.org/1999/xlink"
+ width="26"
+ height="20"
+ role="img"
+ aria-label=""
+>
+ <title></title>
+ <style>
+ a:hover #llink {
+ fill: url(#b);
+ stroke: #ccc;
+ }
+ a:hover #rlink {
+ fill: #4183c4;
+ }
+ </style>
+ <linearGradient id="a" x2="0" y2="100%">
+ <stop offset="0" stop-color="#fcfcfc" stop-opacity="0" />
+ <stop offset="1" stop-opacity=".1" />
+ </linearGradient>
+ <linearGradient id="b" x2="0" y2="100%">
+ <stop offset="0" stop-color="#ccc" stop-opacity=".1" />
+ <stop offset="1" stop-opacity=".1" />
+ </linearGradient>
+ <g stroke="#d5d5d5">
+ <rect
+ stroke="none"
+ fill="#fcfcfc"
+ x="0.5"
+ y="0.5"
+ width="25"
+ height="19"
+ rx="2"
+ />
+ </g>
+ <image
+ x="5"
+ y="3"
+ width="14"
+ height="14"
+ xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxu"
+ />
+ <g
+ aria-hidden="true"
+ fill="#333"
+ text-anchor="middle"
+ font-family="Helvetica Neue,Helvetica,Arial,sans-serif"
+ text-rendering="geometricPrecision"
+ font-weight="700"
+ font-size="110px"
+ line-height="14px"
+ >
+ <rect
+ id="llink"
+ stroke="#d5d5d5"
+ fill="url(#a)"
+ x=".5"
+ y=".5"
+ width="25"
+ height="19"
+ rx="2"
+ />
+ <text
+ aria-hidden="true"
+ x="195"
+ y="150"
+ fill="#fff"
+ transform="scale(.1)"
+ textLength="10"
+ ></text>
+ <text x="195" y="140" transform="scale(.1)" textLength="10"></text>
+ </g>
+</svg>
+
+`
+
exports['The badge generator badges with logo-only should always produce the same badge plastic badge, logo-only 1'] = `
<svg
xmlns="http://www.w3.org/2000/svg"
xmlns:xlink="http://www.w3.org/1999/xlink"
- width="35"
- height="28"
+ width="25"
+ height="18"
role="img"
aria-label=""
>
<title></title>
- <g shape-rendering="crispEdges">
- <rect width="35" height="28" fill="#4c1" />
+ <linearGradient id="s" x2="0" y2="100%">
+ <stop offset="0" stop-color="#fff" stop-opacity=".7" />
+ <stop offset=".1" stop-color="#aaa" stop-opacity=".1" />
+ <stop offset=".9" stop-color="#000" stop-opacity=".3" />
+ <stop offset="1" stop-color="#000" stop-opacity=".5" />
+ </linearGradient>
+ <clipPath id="r"><rect width="25" height="18" rx="4" fill="#fff" /></clipPath>
+ <g clip-path="url(#r)">
+ <rect width="0" height="18" fill="#555" />
+ <rect x="0" width="25" height="18" fill="#4c1" />
+ <rect width="25" height="18" fill="url(#s)" />
</g>
<g
fill="#fff"
text-anchor="middle"
font-family="Verdana,Geneva,DejaVu Sans,sans-serif"
text-rendering="geometricPrecision"
- font-size="100"
+ font-size="110"
>
<image
- x="9"
- y="7"
+ x="5"
+ y="2"
width="14"
height="14"
xlink:href="data:image/svg+xml;base64,PHN2ZyB4bWxu"
/>
- <text
- transform="scale(.1)"
- x="230"
- y="175"
- textLength="0"
- fill="#fff"
- font-weight="bold"
- ></text>
</g>
</svg>
diff --git a/badge-maker/lib/make-badge.spec.mjs b/badge-maker/lib/make-badge.spec.mjs
index 933d173dc1..763a87d6d9 100644
--- a/badge-maker/lib/make-badge.spec.mjs
+++ b/badge-maker/lib/make-badge.spec.mjs
@@ -731,7 +731,7 @@ describe('The badge generator', function () {
})
})
- it('social badge, logo-only', async function () {
+ it('for-the-badge badge, logo-only', async function () {
await expectBadgeToMatchSnapshot({
label: '',
message: '',
@@ -741,13 +741,23 @@ describe('The badge generator', function () {
})
})
+ it('social badge, logo-only', async function () {
+ await expectBadgeToMatchSnapshot({
+ label: '',
+ message: '',
+ format: 'svg',
+ logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
+ style: 'social',
+ })
+ })
+
it('plastic badge, logo-only', async function () {
await expectBadgeToMatchSnapshot({
label: '',
message: '',
format: 'svg',
logo: 'data:image/svg+xml;base64,PHN2ZyB4bWxu',
- style: 'for-the-badge',
+ style: 'plastic',
})
})
})
That gives us a test case for each of the 5 styles (flat, flat-square, FTB, social and plastic) with the test data matching the descriptions.
Resolves #10628
It looks like there is no unit test for this. The badges below can used for previewing: