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

Yuhsuan/documentation #2210

Merged
merged 35 commits into from
Aug 18, 2023
Merged

Yuhsuan/documentation #2210

merged 35 commits into from
Aug 18, 2023

Conversation

YuHsuan-Hwang
Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang commented Jul 21, 2023

Description

  1. Closes Annotate low-level scripting actions in various stores with JSDoc #1482.
  • Updated the documentation in the codebase from JSDoc to TSDoc.

  • Added eslint plugin eslint-plugin-tsdoc to check for the required TSDoc format. Incorrect format will result in a compile error.
    To run lint checks:

    npm run check-eslint
    
  • Implemented a documentation website using Docusaurus.
    To build the website locally:

    cd docs_website
    npm install
    npm start
    

    To create a production build locally:

    npm run build
    npm run serve
    

    The website includes:

    • carta-frontend API: API pages are automatically generated from the codebase TSDoc documentation with docusaurus-plugin-typedoc-api. See documentation-guidelines.md “Writing API documentation” for details.
    • Documentation pages: Additional content is generated from markdown files. See documentation-guidelines.md “Writing documentation pages” for details.
    • Searching: The search functionality is implemented using docusaurus-lunr-search. The feature is only available in production builds. Note that files in theme/SearchBar are automatically generated by running npm run swizzle docusaurus-lunr-search SearchBar -- --eject --danger.
    • Prettier formatting: Files can be reformatted by npm run reformat, including markdown files.

    Notes:

    • The root dir is named as docs_website/ instead of docs/. This is because Docusaurus requires a sub dir docs/, and docs/docs/ is confusing.
    • Node v16.14 or above is required. Tried that the build fails with node v14.18.
  • Added CI checks for formatting and deployment of the documentation website. Used cloud service because our servers will be busy with node-v16 and node-v18 tasks.

  • Added Github actions for updating GitHub Pages whenever the dev branch is updated. To be tested after merging the PR.

  1. Closes Additional tutorial entries for code snippets #1556.

    Added code snippet tutorials at carta-frontend/docs/category/code-snippet-tutorial in the documentation website.
    The first two tutorials are revised from the example code snippets in the codebase. Added some new content such as image properties and regions. Please review the content of the pages.

  2. Closes create document for developer guidelines #1873.

    Added developer guidelines at carta-frontend/docs/category/contributing in the documentation website, including topics of developer tips, Github workflow, unit test guidelines, and documentation guidelines. Please review the content of the pages.

Other changes:

  • Updated example snippets for consistency.
  • Use CRA built-in ESLint
  • Fixed unit test ESLint error

Action items after merging the PR:

  • Setup GitHub Pages settings, and test the Github action for GitHub Pages.
  • Check if the “Edit this page” links work.
  • Check if the tutorial link in the snippets submenu work.

Checklist

For linked issues (if there are):

  • assignee and label added
  • ZenHub issue connection, board status, and estimate updated

For the pull request:

  • reviewers and assignee added
  • ZenHub estimate, milestone, and release (if needed) added
  • e2e test passing / corresponding fix added
  • changelog updated / no changelog update needed
  • protobuf updated to the latest dev commit / no protobuf update needed
  • BackendService unchanged / BackendService changed and corresponding ICD test fix added

Copy link
Collaborator

@ajm-ska ajm-ska left a comment

Choose a reason for hiding this comment

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

@YuHsuan-Hwang This is a very impressive looking. Nice work!
It looks like it should work once merged to the 'dev' branch.

Copy link
Collaborator

@confluence confluence left a comment

Choose a reason for hiding this comment

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

This looks good! A note about deployment: we will need to version this, and have a site for each release as well as the development branch (because users will need to match the API docs to their CARTA release).

@ajm-ska
Copy link
Collaborator

ajm-ska commented Jul 26, 2023

Here is a static build of the frontend documentation for reviewing: https://carta.asiaa.sinica.edu.tw/carta-frontend

@kswang1029
Copy link
Collaborator

@YuHsuan-Hwang when I run npm run check-eslint I see an error as

/Users/kswang/carta_build/carta-frontend/src/components/Shared/ExportImageMenu/ExportImageMenuComponent.test.tsx
9:9 error 'mockModifierString' is assigned a value but never used @typescript-eslint/no-unused-vars

✖ 1 problem (1 error, 0 warnings)

do we expect to see this in this branch?

@kswang1029
Copy link
Collaborator

doing npm install in docs_website folder results in some WARN. Just in case these are important.

npm WARN deprecated stable@0.1.8: Modern JS already guarantees Array#sort() is a stable sort, so this library is deprecated. See the compatibility table on MDN: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#browser_compatibility
npm WARN deprecated trim@0.0.1: Use String.prototype.trim() instead
npm WARN deprecated mkdirp@0.3.0: Legacy versions of mkdirp are no longer supported. Please update to mkdirp 1.x. (Note that the API surface has changed to use Promises in 1.x.)

added 1154 packages, and audited 1155 packages in 17s

Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

Looks great. 👍

Following are my minor comments:

  1. would be good to add links to the API document and code snippets tutorials in the "Snippers sub-menu"
  2. in the code snippets tutorial, it would be good to add "loops" and "conditions" to the "Basics" section.

@YuHsuan-Hwang
Copy link
Collaborator Author

when I run npm run check-eslint I see an error as

/Users/kswang/carta_build/carta-frontend/src/components/Shared/ExportImageMenu/ExportImageMenuComponent.test.tsx
9:9 error 'mockModifierString' is assigned a value but never used @typescript-eslint/no-unused-vars
✖ 1 problem (1 error, 0 warnings)

do we expect to see this in this branch?

This is fixed.

doing npm install in docs_website folder results in some WARN. Just in case these are important.

These are from dependencies, so we can not fix it.

  1. would be good to add links to the API document and code snippets tutorials in the "Snippers sub-menu"

Added:
+ Create New Snippet
The link will work after we host it on Github Pages.

  1. in the code snippets tutorial, it would be good to add "loops" and "conditions" to the "Basics" section.

I don't think we need to add more "JS tutorials" which could be endless ... . I added: "For more usages of ES6-based JavaScript, please refer to the features of the language." in the end of the section.

@kswang1029 Do you think we need to revise the example snippets based on the changes?

@kswang1029
Copy link
Collaborator

when I run npm run check-eslint I see an error as

/Users/kswang/carta_build/carta-frontend/src/components/Shared/ExportImageMenu/ExportImageMenuComponent.test.tsx
9:9 error 'mockModifierString' is assigned a value but never used @typescript-eslint/no-unused-vars
✖ 1 problem (1 error, 0 warnings)

do we expect to see this in this branch?

This is fixed.

doing npm install in docs_website folder results in some WARN. Just in case these are important.

These are from dependencies, so we can not fix it.

  1. would be good to add links to the API document and code snippets tutorials in the "Snippers sub-menu"

Added: + Create New Snippet The link will work after we host it on Github Pages.

  1. in the code snippets tutorial, it would be good to add "loops" and "conditions" to the "Basics" section.

I don't think we need to add more "JS tutorials" which could be endless ... . I added: "For more usages of ES6-based JavaScript, please refer to the features of the language." in the end of the section.

@kswang1029 Do you think we need to revise the example snippets based on the changes?

yes would be good to do so

@YuHsuan-Hwang
Copy link
Collaborator Author

Updated example snippets for consistency.

@YuHsuan-Hwang YuHsuan-Hwang merged commit 5652a81 into dev Aug 18, 2023
12 checks passed
@YuHsuan-Hwang YuHsuan-Hwang deleted the yuhsuan/documentation branch August 18, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants