Update UG Node.js/npm instructions #2589
Conversation
There was a problem hiding this comment.
Thanks for the PR!
Regarding the Node LTS Error, it has already been removed, and it is viewable on the latest documentation linked in the repository readme.
The MarkBind website for the current MarkBind release is at https://markbind.org/.
The website for the latest master branch (not yet released to the public) is at https://markbind-master.netlify.app/.
The last time the MarkBind website was updated was on the date of the latest MarkBind release, so there is a difference between the two, although I should have edited the relevant portion in the user guide with the PR that fixed the issue.
For guidance on how to install npm / node, maybe you could explore the use of a expandable panel, since it would be optional (but good to know) information
Is there any purpose behind the rephrasing of the instructions? Seems slightly unnecessary
| The versions should match or exceed the requirements mentioned in the Prerequisites. | ||
|
|
||
| --- | ||
|
|
There was a problem hiding this comment.
As this section is applicable to only some readers, perhaps put it inside a collapsed panel or a modal?
There was a problem hiding this comment.
Thanks for the review! @gerteck also suggested the use of an expandable panel so I'll implement it that way
|
Thanks for clarifying about the Node LTS error issue @gerteck! I think the expandable panel is a great suggestion, I shall add that in 😄 . Regarding the rephrasing, I was aiming to make the instructions feel less intimidating for beginners, but if you think it doesn’t add much value, I can revert or tweak it. Just let me know! |
lhw-1
left a comment
lhw-1
left a comment
There was a problem hiding this comment.
I think these changes are overall quite useful!
While we are looking at this section, I'd like to suggest that we add a link for this as well:
Thanks for reviewing @lhw-1 ! That’s a great suggestion—I agree that adding a link here would be useful. I’m thinking of using a popover with a message like "Check out this guide". Let me know if you have any thoughts on this approach! |
We could just link it without needing a popover to match the other links (popover is used for |
lhw-1
left a comment
lhw-1
left a comment
There was a problem hiding this comment.
LGTM, thanks @seah-minlong!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2589 +/- ##
=======================================
Coverage 51.86% 51.86%
=======================================
Files 127 127
Lines 5474 5474
Branches 1201 1201
=======================================
Hits 2839 2839
Misses 2340 2340
Partials 295 295 ☔ View full report in Codecov by Sentry. |
lhw-1
changed the title
|
Welcome, @seah-minlong! 🎉 Thank you for your contribution to the MarkBind project! @lhw-1, please remember to add @seah-minlong as an official contributor to our repository. See the full list of contributors here. ✨ |
|
@lhw-1 Each PR must have a SEMVER impact label, please remember to label the PR properly. |
|
@all-contributors please add @seah-minlong for doc |
|
I've put up a pull request to add @seah-minlong! 🎉 |
4 participants
What is the purpose of this pull request?
Fixes #2575
Overview of changes:
Anything you'd like to highlight/discuss:
The Node LTS Error mentioned in #2575 has been fixed in PR #2572, but the website MarkBind Getting Started page has not been updated to reflect this change yet.
Testing instructions:
Proposed commit message: (wrap lines at 72 characters)
Add installing node.js and npm instructions to Getting Started UG
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major,r.Minor,r.Patch.Breaking change release note preparation (if applicable):