-
-
Notifications
You must be signed in to change notification settings - Fork 4.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
Add snarkdown w/ npm auto-update via single package.json #11830
Conversation
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.
Oops 😨 d2681e0 CI test failed ❗
@pyvkd please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/12583 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers
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.
Oops 😨 57c5e83 CI test failed ❗
@pyvkd please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/12592 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers
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.
Oops 😨 30fc132 CI test failed ❗
@pyvkd please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/12612 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers
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.
@pyvkd congratulations! 49823de CI test passed! ✅
Please wait for the further review from the maintainers!
For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12617, thank you 😀
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.
@pyvkd Thank you for your contribution.
I think this PR is good, except some parts.
May I ask you how do you send this pull request? via GitHub website or use git command?
I think the commit should squash to one commit, if you use git command, we can help you how to do this.
But if you use GitHub website, it can't squash the commit. I can help you squash to one commit.
BTW, the commit message needs to be as the followings:
Add snarkdown w/ npm auto-update via single package.json
close #11827, cc developit
Thank you.
Memo: the file of v0.5.0~v1.0.0 I found there is no files can use in web front-end, so it does not matter.
ajax/libs/snarkdown/package.json
Outdated
"description": "Transform Markdown into HTML.", | ||
"authors": [ | ||
"Jason Miller <jason@developit.ca>" | ||
], |
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.
Let's modify from A to B. Because there is one author not authors.
- A
"authors": [
"Jason Miller <jason@developit.ca>"
],
- B
"author": "Jason Miller <jason@developit.ca>",
Thank you.
ajax/libs/snarkdown/package.json
Outdated
"html", | ||
"parser" | ||
], | ||
"filename": "snarkdown.min.js", |
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.
Let's move "filename": "snarkdown.min.js",
between name
and description
field.
Because we'd like let people know what file they need it in the first glance.
Thank you.
@pyvkd I have a question. If we can't use it in web front-end, we need to change the auto-update config. |
@sufuf3 take a look: https://philipwalton.com/articles/deploying-es2015-code-in-production-today/ <!-- Browsers with ES module support load this file. -->
<script type="module" src="main.js"></script>
<!-- Older browsers load this file (and module-supporting -->
<!-- browsers know *not* to load this file). -->
<script nomodule src="main-legacy.js"></script> It may need some more attribute, but anyway, it should be minified. |
Got it, thank you. |
@sufuf3 i used github.com. I can use git command line as well. |
@pyvkd I think the most easiey way here is to keep using GitHub GUI, just fix the issues and we can squash the commits here for you. |
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.
Oops 😨 5e2fa6f CI test failed ❗
@pyvkd please take a look at CI build https://ci.cdnjs.com/cdnjs/cdnjs/12705 for details 📝 !
Most of the error will have corresponding explanation, so that you will know what's wrong and then try to fix it!
If you cannot understand the error message and need help, feel free to ask our maintainers
I'll squash and rebase the branch now. |
5e2fa6f
to
ef9d44e
Compare
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.
@pyvkd congratulations! ef9d44e CI test passed! ✅
Please wait for the further review from the maintainers!
For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12709, thank you 😀
Ping @cdnjs/intern3 |
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.
@pyvkd congratulations! 223f9f9 CI test passed! ✅
Please wait for the further review from the maintainers!
For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12862, thank you 😀
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.
Sorry for the late review.
Helped modify the commit message.
LGTM
@cdnjs/active-contributor @cdnjs/intern3 |
223f9f9
to
f74149b
Compare
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.
@pyvkd congratulations! f74149b CI test passed! ✅
Please wait for the further review from the maintainers!
For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/12933, thank you 😀
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.
LGTM
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.
@sufuf3 @extend1994 so the snarkdown.es.js
was verified working for browser but not for nodejs only?
Any updates here? Please let me know if you need any help. Thanks! (This is an automatic ping message, sorry for disturbing, we will get back here ASAP.) cc @sashberd |
As the mention developit/snarkdown#39 (comment) & #11830 (comment) @PeterDaveHello would you please take a look? |
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.
Please note that the filename needs no .min
as it's minified.
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.
@pyvkd congratulations! 868fa02 CI test passed! ✅
Please wait for the further review from the maintainers!
For the details 📃, please take a look at ➡️ https://ci.cdnjs.com/cdnjs/cdnjs/20300, thank you 😀
@sufuf3 @extend1994 please take a look at my "approval" review, just a small issue and I fixed here. |
close #11827, cc @developit
Profile of the lib