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

Documentation #152

Merged
merged 18 commits into from Aug 22, 2022
Merged

Documentation #152

merged 18 commits into from Aug 22, 2022

Conversation

agriffard
Copy link
Member

Add MkDocs

@agriffard
Copy link
Member Author

agriffard commented Aug 16, 2022

Preview: https://orchardcorecommerce.readthedocs.io/en/ag-docs/

Let me know what you think of it and which kind of doc structure we could adopt.

Ex:

  • Getting Started.
  • 1 Readme per feature / library.
  • Releases

@agriffard
Copy link
Member Author

Then, I will be able to create a sub domain like commerce.orchardcore.net pointing to the docs site.

.readthedocs.yml Outdated Show resolved Hide resolved
.readthedocs.yml Outdated Show resolved Hide resolved
mkdocs.yml Outdated Show resolved Hide resolved
src/README.md Outdated
@@ -0,0 +1,76 @@
# Orchard Core Commerce
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you copy the README.md from the repository root? Why? This will be a maintenance problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I copied it as in OC because it becomes the root of the documentation.

Next goals will be to:

  • Split the infos in multiple pages.
  • Make the root Readme lighter. Ex: Just description, badges and link to doc site.

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to keep docs/README.md, it will be the first page of the docs site.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't resolve the conversations yourself, it makes reviewing harder as I have to look through the closed convos.

Check out this issue, looks like you can include files outside of the docs directory using the Snippets extension. Please try that for docs/README.md. I also suggest renaming it to docs/OrchardCore.Commerce.md so when we include readmes for the other libraries (or any future modules) it will be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Snippet works but we can't refer to the root Readme.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you read the specific comment that I've linked in the issue? It explains where you have to put the redirect: inside the md file, not in the mkdocs.yml. You should have a file like this: https://github.com/pawamoy/duty/blob/master/docs/index.md

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Error: MD041: first-line-heading: First line in a file should be a top-level heading. Rule information: https://github.com/DavidAnson/markdownlint/blob/v0.26.2/doc/Rules.md#md041

Copy link
Contributor

Choose a reason for hiding this comment

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

Great! I've disabled markdownlint for the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

F... da rules.

@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why we need the csproj file?
Nothing in it will be built, but it will create a useless NuGet package once #151 is merged.

Copy link
Member Author

@agriffard agriffard Aug 17, 2022

Choose a reason for hiding this comment

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

Could you explain why we need the csproj file?

To make it appear in the solution.

Nothing in it will be built, but it will create a useless NuGet package once #151 is merged.

It shouldn't be built because of the <IsPackable>false</IsPackable>

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately I found out after merge that this breaks dotnet pack, so I have to delete the csproj file after all.
see https://github.com/OrchardCMS/OrchardCore.Commerce/runs/7962504898?check_suite_focus=true#step:4:4202

src/docs/releases/0.0.1.md Outdated Show resolved Hide resolved
@sarahelsaig
Copy link
Contributor

sarahelsaig commented Aug 17, 2022

Thanks, the preview looks nice. Should we also have a workflow for updating this after every merge to main? (does OC have an existing workflow we can steal? 🙂)

I agree with "1 Readme per feature / library.", incidentally I have already added stub readmes for the two existing libraries in #151 , e.g. here. For now "Getting Started" is short ("Setting up your dev environment" in the main readme) so it doesn't need to be separated. But later we can turn it into a separate page as you suggest.

Copy link
Contributor

@sarahelsaig sarahelsaig left a comment

Choose a reason for hiding this comment

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

Please merge from main and see my comment here.

@sarahelsaig sarahelsaig merged commit 5ba997b into main Aug 22, 2022
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