-
Notifications
You must be signed in to change notification settings - Fork 13
feat(web): add web generator #285
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
==========================================
- Coverage 72.12% 70.54% -1.58%
==========================================
Files 117 128 +11
Lines 9984 10926 +942
Branches 597 633 +36
==========================================
+ Hits 7201 7708 +507
- Misses 2780 3211 +431
- Partials 3 7 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎉 The code now dehydrates to the client so it can render without JavaScript! |
Wow 😵💫 and what about codetab |
It rehydrates and runs with JS, but if you don't have JS, you can still view the docs. I used React's SSRing |
@AugustinMauroy and I got search to finally work 🎉 |
024bbea
to
dbfe55d
Compare
@nodejs/nodejs-website @nodejs/web-infra |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@ovflowd Did something change on the main branch that required a fast-forward? I've undone that commit to see if it fixes the deployment. |
Please don't undo the commit lol |
It was just updated vercel permissions that needed to be fixed. |
Ahh I see, apologies. |
I've re-requested review from previous web-team reviewers, since this PR has undergone substantial changes since the last code review comment. I'm pleased to feel that this PR is in a place where it can land with minimal adjustments (where a follow-up tracking improvements can be made, see the list of improvements in the PR description). |
We have the same thing on https://nodejs.org, and I quite like it. |
Can we pin the remaining feedback items to be impl?: And verify previous feedback was not missed accidentaly (I'd recommend @avivkeller going through the numerous comments I and Dario made xD) |
(FYI Im only going to review the code once the remaining features are impl) |
Noted. I'll go through all the comments, and pin this the things still needing to be implemented in the PR description, sorted by priority. |
To some of the comments that needed an additional reply:
These, if determined to be issues, are in https://github.com/nodejs/nodejs.org. Please open an issue there, if not already. Any changes there will be reflected in the
Have we reached consensus on whether or not to implement this? Perhaps as a
I don't think that's really needed, do you insist?
Do you want me to just copy the current navbar from https://github.com/nodejs/nodejs.org? Or, should this fetch https://github.com/nodejs/nodejs.org/blob/main/apps/site/navigation.json#L2.
I'm still not sure how this would look? Would it be like a signature code block that's just MyClass extends MySuperClass
I've upped the depth.
This isn't an issue with this generator, so IMO it shouldn't block it. See #326 |
Fixes #7.
This PR adds the web generator.
Tasks / Issues
P1 – Must Complete Before Merge
P2 – Must complete before migration
extends
keywordentry.changes
aren't converted to HTML #326P3 – Can Be Done in a Follow-up
mustache
dependencyDataTag
Get a preview
Footnotes
Add things as they appear, or leave review comments. ↩ ↩2 ↩3