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

Bringing back Launch Revamp (focused on reference material of CLI and Core library and development guide) #61

Merged
merged 1 commit into from
May 23, 2024

Conversation

CryptoTotalWar
Copy link
Contributor

@CryptoTotalWar CryptoTotalWar commented May 17, 2024

Description

Issues Fixed

Tasks

  1. May start working on a new branch for the getting-started tickets once this PR is merged.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@CryptoTotalWar CryptoTotalWar self-assigned this May 17, 2024
@CMCDragonkai
Copy link
Member

@CryptoTotalWar PR spec should be implementation oriented, while issue spec should be requirements oriented. The tasks above are a bit bare, while the issue spec is too much.

@CryptoTotalWar
Copy link
Contributor Author

Summary of npmlintfix Issues:

I encountered persistent ESLint configuration issues that appear to stem from settings in the .npmrc file, specifically concerning the prefix configuration which cannot be changed and has caused npm command failures. Despite these setup issues, I've verified that the errors flagged by ESLint in the sidebars.ts file are minor and do not affect the functionality or performance of the site.

Problems in sidebars.ts:

  1. Missing Commas After Arrays: ESLint flagged missing commas after empty arrays in object properties (items: []). These are minor formatting concerns that adhere to JavaScript object syntax standards but do not impact functionality.

  2. Improper Line Breaks: A single line item array was suggested to be placed on a single line to enhance readability and adhere to code style guidelines (items: ['some-item'],).

These ESLint flags are primarily about maintaining consistent coding styles and formatting for better readability and maintainability. However, they do not impact the actual performance or functionality of the documentation site.

@CMCDragonkai
Copy link
Member

The .npmrc is now deprecated in node v20. So it should removed from all repos using node v20.

@CMCDragonkai
Copy link
Member

When linting, just do npm run lintfix to auto fix any linting problems.

@CryptoTotalWar
Copy link
Contributor Author

When linting, just do npm run lintfix to auto fix any linting problems.

I did and it seemed to have auto fixed those minor issues but the problem messages were not disappearing. I guess we just ignore this.

Anyway @CMCDragonkai let me know once you've had a chance to review this PR or if you would like for me to hop online so we can review together.

@CMCDragonkai
Copy link
Member

Some problem messages require a manual fix. In those cases the program just tells you what the problem is. Some things can be autofixed.

Copy link
Member

Choose a reason for hiding this comment

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

This is for all the reference material under polykey-core.

So the basic idea here is that we wouldn't want to replicate everything that gets autogenerated by the docs generator npm run docs such as in https://github.com/MatrixAI/Polykey/tree/staging/docs (https://matrixai.github.io/Polykey/).

So as this is a "human maintained" reference we should:

  1. ALWAYS link to the autogenerated docs like https://matrixai.github.io/Polykey/modules/gestalts.html for the gestalts domain
  2. Then focus on only the human understood reference

For 2., I believe we should focus on these things in order:

  1. Important Types and Data structures
  2. Important Objects and Classes
  3. Important Architecture

Remember the important thing is to focus on "usage" - not on internal details. The docs should then be read by someone intending to use the gestalts domain of Polykey core, not necessarily the developer of the gestalts domain. Cause that way we can separate external API requirements from internal implementation details. And if we avoid specifying too much details, we also avoid having to always change this if we change implementation stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to check/compare with the latest deployed version for reference-polykey-core. I think the version i copied over from the feature-launch-revamp branch a bit brittle or outdates as you said.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were some substantial changes made for some of the reference material under polykey-core.

So what i want to do is the following:

#1 - Verify that the incoming changes are better than what was already there (which what is currently live too) - @CMCDragonkai

#2 - after this i can compare the reference material under polykey-core. with the autogenerated docs material to evaluate duplicate information and retain more high-level information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually i just saw the comparison for gestalts and i think i understand much better what you were saying and i think i know how to action and edit this now... once i make the Push fixing this i will ask you to review for confirmation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#65

for adding the link to the autogenerated docs for each of the pages i created this issue.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need a separate issue for this. This is too simple. It would taken less time to just do it.

images/rpc-1.png Outdated Show resolved Hide resolved
@CMCDragonkai
Copy link
Member

I'm going to comment about implementation problems here.

Regarding:

convert excalidraw docs to svgs

You ticked this off in #52 (comment) but where is this? I don't see any images extracted from excalidraw?

@CMCDragonkai CMCDragonkai changed the title polykey-docs: feature-branch-revamp-manual-fix Bringing back Launch Revamp (focused on reference material of CLI and Core library and development guide) May 20, 2024
@CMCDragonkai
Copy link
Member

Title of PR was renamed to focus on implementation.

@CMCDragonkai
Copy link
Member

What happened to these files?

image

@CryptoTotalWar
Copy link
Contributor Author

The .npmrc is now deprecated in node v20. So it should removed from all repos using node v20.

So is the solution required here to delete the .npmrc file all together?

@CryptoTotalWar
Copy link
Contributor Author

What happened to these files?

image

These were files that were already included in the latest PR. So i checkmarked them. Should have been more explicit about this.

@CryptoTotalWar
Copy link
Contributor Author

I'm going to comment about implementation problems here.

Regarding:

convert excalidraw docs to svgs

You ticked this off in #52 (comment) but where is this? I don't see any images extracted from excalidraw?

That's because i noticed that the excalidraw docs in question were already included.

@CMCDragonkai
Copy link
Member

The .npmrc is now deprecated in node v20. So it should removed from all repos using node v20.

So is the solution required here to delete the .npmrc file all together?

Remove it from this PR.

@CMCDragonkai
Copy link
Member

What happened to these files?
image

These were files that were already included in the latest PR. So i checkmarked them. Should have been more explicit about this.

But were they any different? Did you check the diff?

@CMCDragonkai
Copy link
Member

I'm going to comment about implementation problems here.
Regarding:

convert excalidraw docs to svgs

You ticked this off in #52 (comment) but where is this? I don't see any images extracted from excalidraw?

That's because i noticed that the excalidraw docs in question were already included.

From my understanding, synthesising information from excalidraw is to extract out subsections and put them into the docs directly, not as just uploading the file to the files/ directory. Was this done?

@CMCDragonkai
Copy link
Member

@CryptoTotalWar can you address my review comments above asap starting from here: #61 (review).

I want to have this merged before EOD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Polykey-Docs CLI Tutorial: Feature-Launch-Revamp-Fix
2 participants