-
-
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
📦 version 3 #4756
📦 version 3 #4756
Conversation
* validate input to create()
- Remove use of the doT template library and move to generating SVG output using javascript template literals. - Drop SVGO and mostly manually implement the optimisations. - Add a bunch more tests Co-authored-by: Paul Melnikow <github@paulmelnikow.com>
* drop raster support in package CLI * update docs
'gh-badges' --> 'badge-maker'
whoops - missed this in #4523
This change is only tangentially related We've used the shields repo as an example for these tests so moving files around in our repo has a knock-on effect on them
"name": "badge-maker", | ||
"version": "3.0.0-rc1", |
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've already published a package that does nothing at https://www.npmjs.com/package/badge-maker just to stake a claim on the name.
- I'm suggesting the first release is
-rc1
here, but I don't really have strong opinions on whether it should be-beta
or whatever..
|
Only a handful of service test failures that were unrelated 👍 |
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.
Few minor items left inline, but LGTM and happy to re-👍 if you decide to make any other changes.
Definitely helps that this was done in chunks like you said, as I'd forgotten how fun a few of those earlier commits like f3f95b7 were 😁
Whoops - have updated with a fix. |
I've slightly lost track of what's deployed at this point, but are we pretty happy we've now ironed out the kinks with this? |
The live one is 4999e55. I think we're confident it doesn't have major bugs. I'm okay with merging this. Has anyone evaluated how s0 is performing compared to historical? |
Cool, thanks. In that case I'm going to crack on and merge this, push the release candidate and review the next steps.. |
After merging this PR I got an email from Heroku: "Automatic deployment of shields-staging failed". |
Hmm. Thanks - good spot. The error it is failing with is
I wonder if there is some cache that needs to be cleared? The staging PRs were deploying correctly. |
I've set |
There's a lot going on in this pull request. Fortunately, because we've worked on it in small chunks, most of it has already been reviewed :)
Commits edbad19, cf3612d, 544f4e6, f3f95b7 and fc16600 have all been reviewed already. The relevant PRs are linked in the commit messages, although they've now been though some rebasing to get the branch up to date with master.
Commit 2c02630 and onwards are all new in this PR, but its mostly moving stuff about and a final review/update on the docs.
The next steps I think we should make on this are:
but happy to discuss it.
Just to review the roadmap/issues:
We can make those changes later/in a feature release without breaking backwards compatibility.
Usual review process aside, it would be particularly useful to get another set of eyes on the documentation to make sure we're all up-to-date and the migration advice in the changelog is correct.