-
Notifications
You must be signed in to change notification settings - Fork 432
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
Adding MarkBind as an alternative docs option #156
Conversation
Click here to submit a new iteration when this PR is ready for review. See this repository's contribution guide for more information. |
Hi @damithc, a quick status update:
|
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.
Thanks for this initiative @tlylt
Added a few minor comments.
For the time being (i.e., while this remains experimental), I can provide that information through the module website and se-edu/guides. |
Thanks @damithc for the review, I have updated the docs accordingly.
Will also ask the rest of the MarkBind team to take a look to see if there are further edits needed. |
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.
Thanks @tlylt for the hardwork in migrating the tp docs! Added some preliminary comments. Also, there seems to be an error in the syntax highlighting of the code blocks under tutorials/
, more details in my comments.
docs/Documentation.md
Outdated
* To learn how set it up and maintain the project website, follow the [_**MarkBind User Guide**_](https://markbind.org/userGuide/gettingStarted.html). | ||
* Quick start: | ||
* `cd docs # go to the docs folder` | ||
* `npm i # install dependencies, only needed once when setting up the project` |
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.
How about npm ci
instead?
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.
If npm ci
is used, then having a pin on the v4 version instead of the specific v4.1.0 may not make sense #156 (comment)
Because npm ci will not update the version of markbind-cli. Thoughts?
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.
Right, then should we remove the package-lock
file since that would get overwritten after npm i
anyway?
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.
hmm, it will only be overridden when there are changes to the version downloaded? If we remove it, do you think we should gitignore the package-lock? I'm not sure if that's a common practice.
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 would actually prefer pegging the version to a specific release for stability, and hence use npm ci
. Tagging @damithc for comment on this.
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 would also prefer to peg to a specific release as future releases may introduce bugs. Using npm i
may be risky.
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.
will change to npm ci for now
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.
Numbering issue
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.
Added a few more minor comments.
Hi @damithc, I left some comments replying to your queries. Can let me know what you think when you have time? I think I'm still keen to follow up on this and finish it by this semester. Will also do a final upgrade of the markbind version when v5 is released. |
Sorry @tlylt , didn't realize you are waiting for my feedback: A few other minor things: UG and DG pdf version seems slightly shorter (i.e., fewer pages) in this version, which is good. The current AB3 uses orange color for headings. Should we do something similar in MarkBind version too (to make it look better, and also to show how to customize styles)? And yes, get inputs from other MarkBind devs too. For boxes, I'm wondering if the In the end, we want to make the MarkBind version to look better (and editing not harder) than the current version or else there would be no incentive to switch. |
@damithc I have updated the markbind version required, and I think I have addressed the issues you brought up previously. As for making it more aesthetically pleasing than the current version: I think it looks fine to me as is......perhaps we can go with listing it as an alternative to see how it goes? I won't want to delay this further :) Remaining TODOs listed in the PR description, and you can also preview the deployed site here: https://tlylt.github.io/ab3-docs-demo/index.html 🙆♂️ |
@tlylt Yup, let's get this sorted out before the next semester starts. I'll take a closer look soon. In the meantime, a few things I noticed:
|
Let's migrate the docs site from Jekyll to MarkBind. Primary author: @tlylt in se-edu/addressbook-level3/pull/156 Further tweaks: @damithc in se-edu/addressbook-level3/pull/206
Let's migrate the docs site from Jekyll to MarkBind. Primary author: @tlylt in se-edu/addressbook-level3/pull/156 Further tweaks: @damithc in se-edu/addressbook-level3/pull/206
Let's migrate the docs site from Jekyll to MarkBind. Primary author: @tlylt in se-edu/addressbook-level3/pull/156 Further tweaks: @damithc in se-edu/addressbook-level3/pull/206
Let's migrate the docs site from Jekyll to MarkBind. Primary author: @tlylt in se-edu/addressbook-level3/pull/156 Further tweaks: @damithc in se-edu/addressbook-level3/pull/206
Fixes MarkBind/markbind#2072
Pending:
Finally:
tlylt:markbind-docs
tose-edu:markbind-docs