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

Dev docusaurus #104

Merged

Conversation

bisht-richa
Copy link
Contributor

Description

Fix #XXX

Test

To test this pull request, you can run the following commands:

Additional Information

Tradeoff

Potential improvement

@bisht-richa bisht-richa requested a review from a team as a code owner April 21, 2022 08:02
@viccuad viccuad self-requested a review April 26, 2022 12:00
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

Gave it a spin locally, it's wonderful, looking forward for it to land. I suppose it will land after the other PRs.
Maybe it makes sense to separate commits more granularly, into a commit that adds new files, a commit that changes filepath of existing content, and a commit that changes things inside of the new content. That way, a git rebase main from this branch may be easier. But this is all personal preference for updating this PR down the line.

Copy link
Member

@jvanz jvanz left a comment

Choose a reason for hiding this comment

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

I just ran the code locally and the new version is great!

I have only one question. Should we push the node_modules directory ? It seems something that should not be pushed to the remote. I think the developers can call npm install at the first time and move on. No need to commit this. I'm asking this because in my local env I had to remove the directory and call npm install again.

However, if this is a best practice in the npm world or if you will do more changes, ignore this comment. ;)

docusaurus.config.js Outdated Show resolved Hide resolved
docs/tasks.md Outdated
@@ -17,7 +17,7 @@ The Kubewarden Policy Hub hosts policies contributed by the community. For examp

As shown in the picture below, once you find the policy to be tested, you can copy the registry path<sup>1</sup> or download<sup>2</sup> the `Wasm` binary containing the policy and additional metadata:

![Kubewarden Policy Hub](/img/tasks-policy-hub.png)
![Kubewarden Policy Hub](/images/tasks-policy-hub.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't build it because it doesn't find this image

Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

I run it locally and I love it!! I had a few problems building the site:

SyntaxError: /home/raul/Development/kubewarden/docs-1/docs/writing-policies/spec/04-mutating-policies.md: Unexpected token, expected "}" (139:11)

To fix it I had to remove this table

Also I had to remove this image, as it couldn't be found.

Not sure if I'm doing something wrong or I'm missing something here, I just run yarn install and then yarn start

@ereslibre
Copy link
Member

Thank you! It looks beautiful and well organized. I have the same couple of issues @raulcabello points to. I'm also running it through yarn install and yarn start.

I went through all the pages and I found some small issues, not sure how you want them to be reported.

@divya-mohan0209
Copy link
Contributor

divya-mohan0209 commented Apr 29, 2022

The issue with the mutating page s because I re-added it and we might need to format it to fit markdown perfectly. @raulcabello & @ereslibre : thank you for flagging it. @ereslibre : You can either let me know here itself.

README.md Outdated
@@ -1,2 +1,33 @@
This repo contains the source of the Kubewarden documentation, the rendered manual
can be found [here](https://docs.kubewarden.io/).
# harvesterhci.io
Copy link
Member

Choose a reason for hiding this comment

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

Update Harvester reference. ;)

className: "navbar__docs",
},
{
href: "https://github.com/harvester/harvester",
Copy link
Member

Choose a reason for hiding this comment

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

Update Harvester reference. ;)

package.json Outdated
@@ -0,0 +1,43 @@
{
"name": "harvesterhci",
Copy link
Member

Choose a reason for hiding this comment

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

Update Harvester reference. ;)

package.json Outdated
]
},
"main": "index.js",
"repository": "https://github.com/harvester/harvester.github.io.git",
Copy link
Member

Choose a reason for hiding this comment

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

Update Harvester reference. ;)

@divya-mohan0209 divya-mohan0209 changed the base branch from main to staging-docusaurus May 5, 2022 17:17
Copy link
Member

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

It looks amazing, thank you! Some comments:

  • Here there's a command that has to be boxed (at the end of the screnshot)

Screenshot 2022-05-06 at 09 08 48

  • I think we could pretty format and color YAML files where applicable

Screenshot 2022-05-06 at 09 09 45

I understand that we cannot do so when YAML files are embedded in a kubectl apply -f - command, but that's fine, I only mean in this case, where the YAML file appears alone.

  • Along the lines of the previous point, does docusaurus support Rust highlighting?

Screenshot 2022-05-06 at 09 11 11

  • I think the protocol contracts would be more readable with a monospaced font

Screenshot 2022-05-06 at 09 10 32

Screenshot 2022-05-06 at 09 10 49

Thank you, I love this version!

@viccuad
Copy link
Member

viccuad commented May 6, 2022

I understand that we cannot do so when YAML files are embedded in a kubectl apply -f - command, but that's fine, I only mean in this case, where the YAML file appears alone.

That falls on me, I convinced @jvanz to change all the markdown blocks in that page to ```console, and indeed some of them could be ```yaml, including the ones that start with kubectl apply -f. It would look better.
See: https://github.com/kubewarden/docs/blob/main/src/psp-migration.md?plain=1#L50

Copy link
Member

@ereslibre ereslibre left a comment

Choose a reason for hiding this comment

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

@viccuad viccuad self-requested a review May 6, 2022 09:02
Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @divya-mohan0209 !

@divya-mohan0209 divya-mohan0209 merged commit b2e17c2 into kubewarden:staging-docusaurus May 6, 2022
@divya-mohan0209 divya-mohan0209 mentioned this pull request May 6, 2022
1 task
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.

7 participants