-
-
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
webpage: add missing titles + more badge previews #1305
Conversation
Shouldn't the first commit fail? :) |
I think this was already fixed in #1297. You can see what it looks like here: The tests and the example page are independent. They don't need to match. |
From #1297 (review)
Isn't that more of a workaround instead a fix? ;) From shields-staging.herokuapp.com:
Makes sense, the test itself is still valid. For me it sounds more logical to match them though. |
Eh, it's the same workaround as several of the other long badges. I do think they should all have a title / headline… |
Tried to fix that. Hope I hit all. Please double check, or could you deploy to Heroku review apps? |
lib/all-badge-examples.js
Outdated
previewUri: '/issuestats/p/github/expressjs/express.svg' | ||
}, | ||
{ | ||
title: '(long form)', | ||
title: 'Pull Request Stats (long form)', | ||
exampleUri: '/issuestats/p/long/github/expressjs/express.svg' |
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.
I changed the title here to match what the badge is showing.
Example badge shows: pull closure | 16 hrs
Same here on the PR branch. You might try merging master into this one. Probably one of my recent commits fixed it. To clarify: Working: https://shields-staging.herokuapp.com/ |
|
I'm sorry, but both have the same problem for me. Even after merging most recent badges/shields master to my fork (and this branch). Edit: This is a Firefox and Edge problem... (using FF57 and Edge 41), on chromium it opens the preview! 🙈 Also, #1309 applies to both links and all 3 browsers! |
those example badges are actually not that big compared to others in that category
No empty titles anymore and all preview badges show. Last several comments are basically not relevant anymore due to #1322. Can you revisit this @paulmelnikow? |
I'm fine with the badge changes, though the test change doesn't seem necessary. It's not necessary to keep them perfectly in sync. |
This reverts commit 03389f8.
This simplifies and further optimizes text-width computation by computing the entire width table in advance, and serializing it in the style of QuickTextMeasurer (#1390). This entirely removes the need for PDFKit at runtime. This has the advantage of fixing #1305 – more generally: producing the same result everywhere – without having to deploy a copy of Verdana. The lifting is delegated to these three libraries, which are housed in a monorepo: https://github.com/metabolize/anafanafo I'd be happy to move it into the badges org if folks want to collaborate on maintaining them. QuickTextMeasurer took kerning pairs into account, whereas this implementation does not. I was thinking kerning would be a necessary refinement, though this seems to work well enough. I dropped in a binary-search package to traverse the data structure, in part to conserve space. This causes a moderate performance regression, though there is ample room for improving on that: #2311 (comment)
Node 9 support
(#1290)over
Github rate limits cause transient service test failures in CI
(#979)Was messing around with the layout due to the very wide column needed:
Follow-up commit will fix tests (editing from web).