se-edu / addressbook-level3

:ab::three: Address Book sample application (Level 3)
https://se-education.org/addressbook-level3
MIT License
28 stars 415 forks source link

Adding MarkBind as an alternative docs option #156

Closed tlylt closed 12 months ago

tlylt commented 1 year ago

Fixes https://github.com/MarkBind/markbind/issues/2072

Pending:

Finally:

canihasreview[bot] commented 1 year ago

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

See this repository's contribution guide for more information.

tlylt commented 1 year ago

Hi @damithc, a quick status update:

damithc commented 1 year ago

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.

tlylt commented 1 year ago

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.

tlylt commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

tlylt commented 1 year ago

@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

ok

  • Shall we make the DG part of the siteNav match the actual DG structure, similar to the UG?

Do you mean creating a separate DG page that includes everything? the reason why it looks like this is because the original DG doesn't contain everything, but reference some of the content into their individual pages. Let me know what you think.

image

  • UML diagrams look exactly as before. Upgrade to the new PlantUML version didn't have any visible effect?

Hmm technically the images are only generated once, so I didn't regenerate. Is the original docs going to use the updated images or the old style? If the original docs via Jeykall is going to change then I will change here as well

damithc commented 1 year ago

Do you mean creating a separate DG page that includes everything? the reason why it looks like this is because the original DG doesn't contain everything, but reference some of the content into their individual pages. Let me know what you think.

No, keep the DG as it is, but make the siteNav match the sub-headings in the DG. Perhaps we discussed this earlier and decided against it. In that case it can stay as it is.

BTW, About us should not be here, right? image

Hmm technically the images are only generated once, so I didn't regenerate. Is the original docs going to use the updated images or the old style? If the original docs via Jeykall is going to change then I will change here as well

In the MarkBind version, there should not be a need to generate UML diagrams manually. In fact, that's going to be one of our main selling points for switching to the MarkBind version of the website.

And yes, we will offer it as an opt-in alternative. It will be provided in a separate branch that they can merge if they wish to use the MarkBind version of the website.

tlylt commented 1 year ago

No, keep the DG as it is, but make the siteNav match the sub-headings in the DG. Perhaps we discussed this earlier and decided against it. In that case it can stay as it is.

Oh ok I get what you mean now. So in that case clicking on e.g. Setting up -> will scroll user to the "Setting up, getting started" heading of the DG. Hmm Looking at it again now actually the UG and DG will then have both the site-nav and page-nav be similar, just that the page-nav on the right is more detailed.

BTW, About us should not be here, right? image

Yup I can remove it.

In the MarkBind version, there should not be a need to generate UML diagrams manually. In fact, that's going to be one of our main selling points for switching to the MarkBind version of the website.

I see, I can't remember why this is done like that. Will try the other way.

damithc commented 1 year ago

I see, I can't remember why this is done like that. Will try the other way.

@tlylt let me know when it is done. Currently, some diagrams don't show up in the preview site.

So in that case clicking on e.g. Setting up -> will scroll user to the "Setting up, getting started" heading of the DG. Hmm Looking at it again now actually the UG and DG will then have both the site-nav and page-nav be similar, just that the page-nav on the right is more detailed.

Yup. I think it's OK, as readers can reach a section faster through the SiteNav.

tlylt commented 1 year ago

@tlylt let me know when it is done. Currently, some diagrams don't show up in the preview site.

Hmm the issue is with https://github.com/MarkBind/markbind/issues/1752 where the relative path inclusion of the style.puml doesn't work. I have a fix proposed that should tackle the problem. However, there is still this https://github.com/MarkBind/markbind/issues/1617 issue though... so if i go with this approach, during local development it requires a manual refresh in order for the plantuml images to show up properly in the browser.

Should we still go with this or revert back to including the generated plantuml images?

damithc commented 1 year ago

Hmm the issue is with MarkBind/markbind#1752 where the relative path inclusion of the style.puml doesn't work. I have a fix proposed that should tackle the problem. However, there is still this MarkBind/markbind#1617 issue though... so if i go with this approach, during local development it requires a manual refresh in order for the plantuml images to show up properly in the browser.

Does adding {{ baseUrl }} in front solve this problem? I had a similar issue with image annotation and the baseUrl workaround solved it.

Should we still go with this or revert back to including the generated plantuml images?

It's nice if we have full PlantUML support. It's our biggest selling point for switching from Jekyll to MarkBind. Do you feel like the manual refresh is a major problem when working on AB3 DG? Would it be hard to fix? Perhaps not regenerate the diagram if there is no change to the puml file.

tlylt commented 1 year ago

Hmm the issue is with MarkBind/markbind#1752 where the relative path inclusion of the style.puml doesn't work. I have a fix proposed that should tackle the problem. However, there is still this MarkBind/markbind#1617 issue though... so if i go with this approach, during local development it requires a manual refresh in order for the plantuml images to show up properly in the browser.

Does adding {{ baseUrl }} in front solve this problem? I had a similar issue with image annotation and the baseUrl workaround solved it.

I don't think this works with our existing implementation because the inclusion is from within a .puml file.

I guess It could be a possible solution if we do some code changes wrt how we process .puml files (adding some preprocessing).

It's nice if we have full PlantUML support. It's our biggest selling point for switching from Jekyll to MarkBind. Do you feel like the manual refresh is a major problem when working on AB3 DG? Would it be hard to fix? Perhaps not regenerate the diagram if there is no change to the puml file.

The manual refresh is required even for markbind serve. It's slightly annoying to see the broken images...probably tolerable but not the best.

From the issue description, it seems to not be as straightforward because even if we avoid regenerating, when there are edits to the .puml file, we still need to handle this generation and page update properly. Might take some effort to investigate.

damithc commented 1 year ago

From the issue description, it seems to not be as straightforward because even if we avoid regenerating, when there are edits to the .puml file, we still need to handle this generation and page update properly. Might take some effort to investigate.

For now, we can mention that a 'refresh might be required if UML images appear as a broken link'. Hopefully, we can fix it before students start updating the DG.

tlylt commented 1 year ago

From the issue description, it seems to not be as straightforward because even if we avoid regenerating, when there are edits to the .puml file, we still need to handle this generation and page update properly. Might take some effort to investigate.

For now, we can mention that a 'refresh might be required if UML images appear as a broken link'. Hopefully, we can fix it before students start updating the DG.

Fixed.

Preview: https://tlylt.github.io/ab3-docs-demo/ To play around directly locally, simply checkout this branch

damithc commented 1 year ago

@tlylt While trying to add this option to this batch of CS2103, I noticed that https://markbind.org/userGuide/gettingStarted.html has two sets of commands for the same tasks, and https://tlylt.github.io/ab3-docs-demo/Documentation.html has a third variation. Can we stick the the original version? i.e., npm install -g markbind-cli -> markbind serve Or, is there an advantage in switching to one of the other variations, in which case we should pick one of them and use it everywhere.

tlylt commented 1 year ago

Can we stick the the original version? i.e., npm install -g markbind-cli -> markbind serve Or, is there an advantage in switching to one of the other variations, in which case we should pick one of them and use it everywhere.

Either approach would work fine in this case. It might be more suitable to have local package installation here for ease of version management. This is because we are going to specify markbind-cli as part of the package.json, which will be used to track the change in version, help installation in CI, and also has the added benefit of reflecting the usage back in MarkBind GitHub repo's usage tab.

The approach I mentioned in this docs is not a third variation, it is the alternative approach in the MarkBind docs under:

Alternative installation: as a local dev-dependency with package.json

We had npm install -g markbind-cli for a long time so I didn't propose replacing it when I added the local installation approach and the quick start section initially. I'm in favour of making an update to our MarkBind docs to update the recommendation (npx + local dev-dependency, just like https://docusaurus.io/docs/installation).

damithc commented 1 year ago

I see. Let's pick one set of commands (preferably, the simplest) and use it everywhere in the UG (it is OK to have different set of instructions for devs though -- provided it is doesn't appear in the UG). Also note that we have https://markbind.org/userGuide/cliCommands.html which should be aligned with https://markbind.org/userGuide/gettingStarted.html How should we update the main UG quickly so that users are not shown multiple versions of commands? Once the main UG is updated, I'll update the AB-3 version accordingly.

tlylt commented 1 year ago

I see. Let's pick one set of commands (preferably, the simplest) and use it everywhere in the UG (it is OK to have different set of instructions for devs though -- provided it is doesn't appear in the UG).

Also note that we have https://markbind.org/userGuide/cliCommands.html which should be aligned with https://markbind.org/userGuide/gettingStarted.html

How should we update the main UG quickly so that users are not shown multiple versions of commands? Once the main UG is updated, I'll update the AB-3 version accordingly.

I can make a documentation PR n release it by this week. What do you think?

For the commands in CLI, they are still accurate, it's just that without installing markbind globally, you can't call it directly (perhaps unless it's in the system path, idk). Hence for npx we have to call "npx markbind-cli ..." to invoke our npm entry point package, instead of "npx markbind ...". We can definitely revisit this to see if we want to adjust how we distribute the packages, but I might need second opinions and suggestions from others before proceeding.

damithc commented 1 year ago

@tlylt I need to release the tP repo by Friday. OK to update the main UG page by Sunday, as most teams will not set up their tP immediately. For this round, perhaps we stick to the original global installation method only? i.e., either remove the quickstart box in the UG, or move it to the bottom and frame it as an alternative way to use MarkBind? Otherwise the very first bit of MarkBind UG students see can cause doubts, as the commands given in the quickstart doesn't match the regular instructions. Once we figure out which option to recommend (global or local, or give both options), we can revise the UG (and other relevant pages) accordingly, later.

tlylt commented 12 months ago

@tlylt I need to release the tP repo by Friday. OK to update the main UG page by Sunday, as most teams will not set up their tP immediately. For this round, perhaps we stick to the original global installation method only? i.e., either remove the quickstart box in the UG, or move it to the bottom and frame it as an alternative way to use MarkBind? Otherwise the very first bit of MarkBind UG students see can cause doubts, as the commands given in the quickstart doesn't match the regular instructions. Once we figure out which option to recommend (global or local, or give both options), we can revise the UG (and other relevant pages) accordingly, later.

Sure will make a PR to frame the quickstart as alternative for now.

damithc commented 12 months ago

@tlylt thanks. I also noticed that some of the files in this branch are used for the new approach of installation e.g., package.json Should we still keep them, and if so, how do users use them?

tlylt commented 12 months ago

@tlylt thanks. I also noticed that some of the files in this branch are used for the new approach of installation e.g., package.json Should we still keep them, and if so, how do users use them?

Do you mean changing the approach here to using global installation as well? For how users use this approach in this repo, I actually have quite detailed instructions in https://tlylt.github.io/ab3-docs-demo/Documentation.html (in the quick start panel) within this site.

The students should not need to refer to the markbind installation in the markbind website actually.

I would strongly recommend this because I think the global installation and the local installation (unlike npx), can co-exist. So in this case since students would probably just have one markbind site, keep it locally will help us manage the version in the future and most importantly, it's the part where if we do this package.json, the number of people using our package will increase in our markbind repo, that helps to boost visibility I think:)

Is it because I have this point:

To learn how set it up and maintain the project website, follow the MarkBind User Guide.

in the page? I can remove that to just mention following the guide in the page.

damithc commented 12 months ago

@tlylt my concern with these details instructions is that they don't seem to map directly to any of the three alternatives e.g., none of the alternatives use npm ci. Ideally, we should be able to say "Follow the UG; use alternative X" and be done with it, rather than having to give detailed instructions again (other than any AB3 specific additional instructions). Even if give detailed instructions (for their convenience), they should be a sub-set of one of the alternatives.

tlylt commented 12 months ago

@tlylt my concern with these details instructions is that they don't seem to map directly to any of the three alternatives e.g., none of the alternatives use npm ci. Ideally, we should be able to say "Follow the UG; use alternative X" and be done with it, rather than having to give detailed instructions again (other than any AB3 specific additional instructions). Even if give detailed instructions (for their convenience), they should be a sub-set of one of the alternatives.

Ah I see. This is the local installation approach mentioned in our UG. I can make the adjustment here to refer to it directly.

We don't have the npm ci step because I have already set up the repo based on that instruction, which is meant for the first-time installation. So for subsequent usage, a user only needs to npm ci + npm run serve for the repo.

I will also make it clearer in markbind UG under local installation, do you think that will help?

Sidenote, this local installation approach is being used by teammates: https://teammates.github.io/teammates/documentation.html

damithc commented 12 months ago

I will also make it clearer in markbind UG under local installation, do you think that will help?

@tlylt Yes, let's explain it clearly. Our target users (i.e., content writers who might have a little bit of programming knowledge) would need more handholding than regular Web developers. Let's write assuming the reader has never used npm before.

damithc commented 12 months ago

@tlylt I have taken a copy of this branch and doing some further tweaks. So, don't push any more commits to this branch. Will let you know when my version is ready.

For installing Graphviz on Windows, can just download the installer from https://graphviz.org/download/ ? i.e., rather than use Chocolatey

tlylt commented 12 months ago

Yes should be ok as long as it's installed.

tlylt commented 12 months ago

For installing Graphviz on Windows, can just download the installer from graphviz.org/download ? i.e., rather than use Chocolatey

Btw Windows users no longer need to install Graphviz as per https://markbind.org/userGuide/components/imagesAndDiagrams.html#diagrams just that there will be a warning about missing Graphviz, which we will need to remove in the future.

damithc commented 12 months ago

@tlylt I've created a revised version of this PR #206 containing two commits: one mostly based on this PR, and one mostly based on my refinements. Once finalized, will revised the commit message to give your credit as the primary author. I moved most of the documentation guide to se-edu/guides because: