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

Reorganize MarkBind installation instructions in UG #2366

Merged
merged 3 commits into from Sep 9, 2023

Conversation

tlylt
Copy link
Contributor

@tlylt tlylt commented Sep 9, 2023

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • Feature addition or enhancement
  • Code maintenance
  • DevOps
  • Improve developer experience
  • Others, please explain:

Overview of changes:
Re-organize the installation instructions into three methods. Add a note in CLI command docs.

Request came from: se-edu/addressbook-level3#156 (comment)

Anything you'd like to highlight/discuss:
In the future, we might want to move away from using global installation, which will require adjustments to the setup instructions again.

Testing instructions:
See UG:

Proposed commit message: (wrap lines at 72 characters)
Reorganize MarkBind installation instructions in UG


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No unrelated changes

@codecov
Copy link

codecov bot commented Sep 9, 2023

Codecov Report

Merging #2366 (e984b00) into master (d1adec8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2366   +/-   ##
=======================================
  Coverage   45.16%   45.16%           
=======================================
  Files         123      123           
  Lines        5159     5159           
  Branches     1086     1086           
=======================================
  Hits         2330     2330           
  Misses       2510     2510           
  Partials      319      319           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tlylt tlylt requested a review from damithc September 9, 2023 03:10
@damithc
Copy link
Contributor

damithc commented Sep 9, 2023

@tlylt How about at the very top we mention something like the following?

The steps below explains how to install ... If you prefer to .... instead, please scroll to .... or, if you prefer to ... scroll to ...

And then, we put the two alternatives as separate sections on their own (not as panels), at the bottom. That way, the two alternatives appear as peers of the original rather than being tucked away under step 1 of the original.

Also, in the cli page, do we need to mention how to adjust the commands if they are using the alternatives?

@tlylt
Copy link
Contributor Author

tlylt commented Sep 9, 2023

@tlylt How about at the very top we mention something like the following?

The steps below explains how to install ... If you prefer to .... instead, please scroll to .... or, if you prefer to ... scroll to ...

And then, we put the two alternatives as separate sections on their own (not as panels), at the bottom. That way, the two alternatives appear as peers of the original rather than being tucked away under step 1 of the original.

ok.

Also, in the cli page, do we need to mention how to adjust the commands if they are using the alternatives?

Hmm, I think there's no need because ultimately the CLI is still the same. For global you can invoke markbind directly because it's available globally in your terminal. For local and npx, underlying calls are still markbind serve etc. I can make a note if you think it helps clarify? For users who understand npx and npm packages, I think the confusion is quite minimal.

@damithc
Copy link
Contributor

damithc commented Sep 9, 2023

For users who understand npx and npm packages, I think the confusion is quite minimal.

@tlylt As our target users are not regular node/JavsScript users, it is best to assume no knowledge of node/npm.

@tlylt tlylt changed the title Frame npx installation as an alternative in UG Reorganize MarkBind installation instructions in UG Sep 9, 2023
@tlylt
Copy link
Contributor Author

tlylt commented Sep 9, 2023

The steps below explains how to install ... If you prefer to .... instead, please scroll to .... or, if you prefer to ... scroll to ...

I added a note at the top. For every method, I begin the section with a note about the recommendation. Hope that clarifies.

@damithc
Copy link
Contributor

damithc commented Sep 9, 2023

I added a note at the top. For every method, I begin the section with a note about the recommendation. Hope that clarifies.

@tlylt Looks good. So, I guess option 2 is the one that aligns with AB3? In that case, do we need to update that AB3 branch every time we release a new MarkBind version (due to package.json and package-lock.json)?

@tlylt
Copy link
Contributor Author

tlylt commented Sep 9, 2023

I added a note at the top. For every method, I begin the section with a note about the recommendation. Hope that clarifies.

@tlylt Looks good. So, I guess option 2 is the one that aligns with AB3?

Yes

In that case, do we need to update that AB3 branch every time we release a new MarkBind version (due to package.json and package-lock.json)?

If we want to update to the new version then yes (we don't have to if we don't need the new update urgently). Again, I strongly recommend this approach because pinning the dependency is so much safer than going with option 1, which might just be slightly more convenient.

Some reasons already discussed here se-edu/addressbook-level3#156 (comment)

@damithc
Copy link
Contributor

damithc commented Sep 9, 2023

@tlylt Sounds good. I asked because I anticipate a new release soon (to fix the incorrect scrolling issue) which AB3 adopters need to upgrade as well. On a related note, should we also add one more item under each of the three alternatives, to explain how to upgrade MarkBind later?

@tlylt
Copy link
Contributor Author

tlylt commented Sep 9, 2023

@tlylt Sounds good. I asked because I anticipate a new release soon (to fix the incorrect scrolling issue) which AB3 adopters need to upgrade as well.

Yup, for every necessary update at the moment, I will need to bump the version there 😰.

On a related note, should we also add one more item under each of the three alternatives, to explain how to upgrade MarkBind later?

Added.

@damithc
Copy link
Contributor

damithc commented Sep 9, 2023

Looks good. For method 3, I thought the latest version is used by default? If that's the case, the only thing that needs to be mentioned in item 5 is how to use a specific version, right?

@tlylt
Copy link
Contributor Author

tlylt commented Sep 9, 2023

Looks good. For method 3, I thought the latest version is used by default? If that's the case, the only thing that needs to be mentioned in item 5 is how to use a specific version, right?

Yes if you run it for the first time I believe. Then, there will be a cached version of the package that may or may not be cleared automatically, which will give you a stale version if you don't specify fetching the latest. https://stackoverflow.com/questions/73184553/npx-does-not-look-for-latest-version-of-package

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 the quick fix @tlylt
No further comments from me.

@tlylt tlylt added this to the v5.0.3 milestone Sep 9, 2023
@tlylt tlylt merged commit 4dd8d72 into MarkBind:master Sep 9, 2023
8 checks passed
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.

None yet

2 participants