-
Notifications
You must be signed in to change notification settings - Fork 145
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
refactor: add tbody to contributors table #307
Conversation
Where do you even get that error? Also, it might be worth fixing your PR so the failing CI check passes. |
Using MDX instead of markdown.
Precisely why I'm using WIP in the title... I'll finish this ASAP |
2c7f387
to
2f9440c
Compare
2f9440c
to
d65f793
Compare
I'm failing to understand whats wrong with the CI check though
While package.json states: "chalk": "^4.0.0", And yarn why says:
|
Hi, may I check if there are any updates on this PR? I faced a similar issue lately, the use case is as such:
|
It states that it needs |
I think the v12.17 is referring to the node version, it needs to be 12.17 and above for chalk v5 |
That's true; I misread that. |
6328d33
to
19bb6b0
Compare
19bb6b0
to
164e95d
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.
@hpierre74 Hi, awesome man! validate_and_deploy
is now working again. Please check the requested changes. Thanks! 🎉
src/generate/index.js
Outdated
newContent => { | ||
return `\n<table>\n <tr>\n ${newContent}\n </tr>\n</table>\n\n` | ||
return `\n<table>\n \n<tbody>\n <tr>\n ${newContent}\n </tr>\n </tobdy>\n </table>\n\n` |
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.
@hpierre74 looks like need to write a test for this one
src/generate/index.js
Outdated
@@ -55,9 +55,9 @@ function generateContributorsList(options, contributors) { | |||
}), | |||
_.chunk(options.contributorsPerLine), | |||
_.map(formatLine), | |||
_.join('\n </tr>\n <tr>\n '), | |||
_.join('\n </tr>\n <tr>\n '), |
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.
@hpierre74 also need test for this one
Somehow related to #163 |
@tenshiAMD I'm trying to figure this out as we speak, adding spaces there and there 😄 |
Now there is an unrelated test failing, I suspect this is a side effect of upgrading circleci node version, I'll look into it [test] FAIL src/util/__tests__/url.js (10.5 s)
[test] ● Throw an error when parsed input isn't a URL
[test]
[test] expect(received).toThrowError(expected)
[test]
[test] Expected substring: "Invalid URL: http://some string"
[test] Received message: "Invalid URL"
[test]
[test] 18 | }
[test] 19 |
[test] > 20 | const url = new URL(new RegExp('^\\w+\\:\\/\\/').test(input) ? input : `http://${input}`)
[test] | ^
[test] 21 |
[test] 22 | if (!isHttpProtocol(url.protocol)) {
[test] 23 | throw new TypeError('Provided URL has an invalid protocol')
[test]
[test] at parseHttpUrl (src/util/url.js:20:15)
[test] at Object.<anonymous> (node_modules/expect/build/toThrowMatchers.js:83:11)
[test] at Object.toThrowError (node_modules/expect/build/index.js:338:21)
[test] at Object.<anonymous> (src/util/__tests__/url.js:48:48)
[test] at Object.<anonymous> (src/util/__tests__/url.js:48:48) |
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.
@hpierre74 LGTM however I noticed some changes. Why are some unnecessary lines got changed? Are you using a custom .editorconfig
plugin or something?
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.
@hpierre74 LGTM however I noticed some changes. Why are some unnecessary lines got changed? Are you using a custom .editorconfig
plugin or something? I think we can remove those first then we can merge this one. Cheers! 🎉
🎉 This PR is included in version 6.20.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hi @tenshiAMD, wonder if you know whether this change will propagate to the all-contributor bot immediately or will need to wait for the https://github.com/all-contributors/app side to update? |
@tlylt not totally sure about this but I guess there is a slight delay thus it is always best to wait for few minutes or so. |
Thanks @tenshiAMD ! Just tried it after a few hours and it doesn't seem to be updated: Perhaps the app side needs to update the cli dependency to the latest for this to work... Thanks anyway |
@tlylt referring you to @Berkmann18. He might help us regarding this issue. |
@tlylt I made a PR for this, we are just waiting for @Berkmann18 or @gr2m to merge it. Thanks! 🎉 |
@tlylt can you check again now? Please let us know. Thanks! 🎉 |
Tested and works well! Thank you for the follow-up 💯 |
* origin/master: (85 commits) refactor: log full error stack on error (#316) chore: fix status badges (#315) docs: add JoshuaKGoldberg as a contributor for bug (#314) fix: incorrect usage of `tbody` (#311) fix: trim `nextLink` before slicing (#309) fix: set default value as `7` for `contributorsPerLine` (#139) chore(deps): bump dependencies and devDeps (#298) refactor: add tbody to contributors table (#307) docs: add Lucas-C as a contributor for doc (#306) fix: scriptName + improving usage messages (#305) docs: add vapurrmaid as a contributor (#304) chore(deps): CVE-2021-23337 in inquirer->lodash (#303) docs: add SirWindfield as a contributor (#297) feat: add namespaced token (#296) docs: add LaChapeliere as a contributor (#292) feat(contribution-types): add research contribution type (#291) docs: add darekkay as a contributor (#290) feat: display a meaningful error when the config file is missing (#288) docs: add melink14 as a contributor (#285) docs: add jdalrymple as a contributor (#264) ...
The huge diff is caused by all-contributors/cli#307
The huge diff is caused by all-contributors/cli#307
What: The contributors table generator
Why: Browsers are inserting tbody inside tables when it is not found, it is not really enforced by W3C to add tbody but since MDX will complain because of the react usage beneath it, I think this change has no negative impact and can fix an error in some docusaurus (or other MDX solutions).
FYI: When react reconciles the tree it finds a difference in the markup since browsers add the tbody, leading to an error
How: Simply wrap the
tr
inside a tbody in the generator html stringChecklist: