Skip to content
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

Closed
wants to merge 23 commits into from

Conversation

tlylt
Copy link

@tlylt tlylt commented Feb 9, 2023

Fixes MarkBind/markbind#2072

Pending:

Finally:

  • Instead of merging, someone with write access to this repo checkout and make a copy of tlylt:markbind-docs to se-edu:markbind-docs
  • Some update is required from the CS2103T website to mention this option along side Jeykell, when mentioning tP docs

@canihasreview
Copy link

canihasreview bot commented Feb 9, 2023

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@tlylt
Copy link
Author

tlylt commented Feb 11, 2023

Hi @damithc, a quick status update:

  • listed out the outline of the steps required, and some of the pending TODOs in the PR description
  • Will likely publish a release on the MarkBind side to address the blocker here, so that the rest can ideally be done by next week
  • have deployed a version of the adapted ab3 docs, mostly ready for a read-through/review of the presentation, at https://tlylt.github.io/ab3-docs-demo/index.html
  • for the 2103T side do you have any plan on how instructions need to be updated once this docs work is completed?

Copy link
Contributor

@damithc damithc left a 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.

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/_markbind/layouts/default.md Outdated Show resolved Hide resolved
docs/_markbind/layouts/default.md Outdated Show resolved Hide resolved
docs/UserGuide.md Outdated Show resolved Hide resolved
docs/_markbind/layouts/default.md Outdated Show resolved Hide resolved
.github/workflows/docs.yml Outdated Show resolved Hide resolved
@damithc
Copy link
Contributor

damithc commented Feb 11, 2023

for the 2103T side do you have any plan on how instructions need to be updated once this docs work is completed?

For the time being (i.e., while this remains experimental), I can provide that information through the module website and se-edu/guides.

docs/Documentation.md Outdated Show resolved Hide resolved
@tlylt
Copy link
Author

tlylt commented Feb 14, 2023

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.

Copy link

@raysonkoh raysonkoh left a 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.

* 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`

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?

Copy link
Author

@tlylt tlylt Feb 14, 2023

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?

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?

Copy link
Author

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.

Copy link
Author

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.

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.

Copy link
Author

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

docs/Documentation.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Show resolved Hide resolved
Copy link
Author

@tlylt tlylt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Numbering issue

docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
docs/tutorials/TracingCode.md Outdated Show resolved Hide resolved
Copy link
Contributor

@damithc damithc left a 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.

docs/site.json Show resolved Hide resolved
docs/_markbind/layouts/default.md Outdated Show resolved Hide resolved
docs/_markbind/layouts/default.md Outdated Show resolved Hide resolved
docs/UserGuide.md Show resolved Hide resolved
docs/UserGuide.md Show resolved Hide resolved
docs/Documentation.md Outdated Show resolved Hide resolved
@tlylt
Copy link
Author

tlylt commented Mar 26, 2023

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.

@damithc
Copy link
Contributor

damithc commented Mar 26, 2023

Sorry @tlylt , didn't realize you are waiting for my feedback:

A few other minor things:

Heading level mismatch?
image

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 light or the seamless styles would make the content blend in with the main text more seamlessly. But this is a matter of taste and subjective.

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.

@tlylt tlylt changed the title [WIP] Adding MarkBind as an alternative docs option Adding MarkBind as an alternative docs option Jul 18, 2023
@tlylt tlylt marked this pull request as ready for review July 18, 2023 11:58
@tlylt
Copy link
Author

tlylt commented Jul 18, 2023

@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

🙆‍♂️

@damithc
Copy link
Contributor

damithc commented Jul 18, 2023

@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:

  • The home page should have the project name AddressBook Level-3 at the top
  • Shall we make the DG part of the siteNav match the actual DG structure, similar to the UG?
  • UML diagrams look exactly as before. Upgrade to the new PlantUML version didn't have any visible effect?

lihongguang00 pushed a commit to lihongguang00/tp that referenced this pull request Oct 14, 2023
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
damithc pushed a commit that referenced this pull request Feb 9, 2024
Let's migrate the docs site from Jekyll to MarkBind.

Primary author: @tlylt in /pull/156
Further tweaks: @damithc in /pull/206
xuelinglow pushed a commit to xuelinglow/tp that referenced this pull request Feb 26, 2024
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
damithc pushed a commit to damithc/ab3-markbind that referenced this pull request Aug 8, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CS2103T tP docs migration tracker
3 participants