-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Docs update and restructure #4596
Docs update and restructure #4596
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #4596 +/- ##
==========================================
+ Coverage 72.15% 77.18% +5.03%
==========================================
Files 144 145 +1
Lines 14583 14659 +76
Branches 563 586 +23
==========================================
+ Hits 10522 11315 +793
+ Misses 3931 3233 -698
+ Partials 130 111 -19
Flags with carried forward coverage won't be shown. Click here to find out more.
|
7ef1350
to
778986f
Compare
fb75e52
to
746f1fd
Compare
746f1fd
to
69fe83f
Compare
a14beb1
to
073e85d
Compare
|
||
### 1. Checkout a git branch | ||
<PlatformSelector :selectedPlatform="selectedPlatform" v-on:change="setPlatform" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the docs pure markdown?
This is a small feature, but it's a slippery slope, and we might have to do another BIG change to get out of vitepress later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is much more readable and transparent with those switch buttons. Two separate sections or pages that kinda duplicate each other looks very bulky, not to mention their maintenance. Is there any plans to get rid of Vitepress in the nearest future? For some cases we obviously need something more than plain markdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that this file needs to be pure Markdown too, ideally in Markdown that is applicable to both GitHub-Flavored Markdown and Vitepress (so no Vitepress specific features, and no GFM specific features).
GitHub has a nice UI that tells first-time contributors when they make a PR to read the CONTRIBUTING.md
file, so ideally we'd put everything into that file, and just copy it to Vitepress for the website.
As a workaround, you could instead do something like:
## Setup and Launch
_If you'd prefer to develop in a Docker container, please see [Setup and Launch using Docker](#setup-and-launch-using-docker)_
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
This ensures that the rendering of that feature in the e2e will be reviewed in the release process going forward. Less chance that it breaks! | ||
|
||
To start working with the e2e tests: | ||
::: tip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ```tip, not the :::
one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I faced some issues with it. Only :::
was working properly. I will try it again. Perhaps the problem was with its content. I needed a code block inside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
For a more detailed introduction to Mermaid and some of its more basic uses, look to the [Beginner's Guide](../community/n00b-overview.md) and [Usage](../config/usage.md). | ||
For a more detailed introduction to Mermaid and some of its more basic uses, look to the [Beginner's Guide](../intro/n00b-gettingStarted.md) and [Usage](../config/usage.md). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the n00b
from the file names?
We should setup the redirects in redirects.ts file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided not to redo everything in one batch, that will be done in upcoming requests. Applying changes and simultaneous renaming of a file can cause diff become meaningless.
collapsible: true, | ||
collapsed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why collapse everything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This option did not worked at all, I simply renamed the option, the value left intact. If we want our sidebar to be expanded by default, I'll change its value too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now everything is expanded by default and menu is collapsible. If we don't want to make it collapsible, we need to remove the option entirely
0763a72
to
b757e2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at improving the documentation! I agree with most of @sidharthv96's points, but I have some other thoughts too:
My gut feeling is that our README.md
file should be exactly the same (or as similar as possible) to the packages/mermaid/src/docs/intro/index.md
file.
I also feel like the CONTRIBUTING.md
should be as similar as possible to the packages/mermaid/src/docs/community/x
file.
Does it make sense to combine the:
packages/mermaid/src/docs/community/code.md
,packages/mermaid/src/docs/community/development.md
, andpackages/mermaid/src/docs/community/documentation.md
files into a single big packages/mermaid/src/docs/community/CONTRIBUTING.md
file, that we can then symlink or copy to CONTRIBUTING.md
?
Btw, there are a bunch of TODO
messages in the documentation. Are those intended to be there? If yay, we might want to mark this PR as a draft until they're all fixed.
|
||
### 1. Checkout a git branch | ||
<PlatformSelector :selectedPlatform="selectedPlatform" v-on:change="setPlatform" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that this file needs to be pure Markdown too, ideally in Markdown that is applicable to both GitHub-Flavored Markdown and Vitepress (so no Vitepress specific features, and no GFM specific features).
GitHub has a nice UI that tells first-time contributors when they make a PR to read the CONTRIBUTING.md
file, so ideally we'd put everything into that file, and just copy it to Vitepress for the website.
As a workaround, you could instead do something like:
## Setup and Launch
_If you'd prefer to develop in a Docker container, please see [Setup and Launch using Docker](#setup-and-launch-using-docker)_
b757e2c
to
a77293c
Compare
7e3ade8
to
7fa49f2
Compare
Yes, it does. I think it will be put in order in a separate request.
No, this is not a draft. Almost all of those TODO's are current part of documentation. They will be addressed in a separate batch of changes |
7fa49f2
to
0b8bb71
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks better!
I've figured out why the ![]()
... shields in packages/mermaid/src/docs/intro/index.md
wasn't working, see fix in #4596 (comment)
Does it make sense to combine the:
packages/mermaid/src/docs/community/code.md
,packages/mermaid/src/docs/community/development.md
, andpackages/mermaid/src/docs/community/documentation.md
files into a single big
packages/mermaid/src/docs/community/CONTRIBUTING.md
fileYes, it does. I think it will be put in order in a separate request.
If you can, sticking it in this PR would make the git diff
a lot simpler, and so:
- This would make the PR easier to review.
- It would make merge conflicts less likely to happen.
But I know using git rebase
to clean up commits can be pretty difficult, and this is just documentation, not code, so it's not too big of a deal if you don't want to do it.
I think we should at least definitely combine the packages/mermaid/src/docs/community/development.md
and packages/mermaid/src/docs/community/docker-development.md
files, since those two are very similar (and the docker-development.md
file has some custom Vitepress YAML front-matter that will be difficult to keep in sync with the sidebar).
prev: | ||
text: 'FAQ' | ||
link: 'config/faq' | ||
next: | ||
text: 'Contributing Code' | ||
link: '/community/code' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be easier to just have the Docker instructions in the same file as the non-docker instructions?
E.g. something like:
## Get the Source Code
## Technical Requirements
> The following documentation describes how to work with Mermaid in your host
> environment. There's also a
> [Docker installation guide](#docker-development) if you prefer to
> work in a Docker environment.
## Setup and Launch
### Switch to project
### Install packages
### Launch
## Verify Everything is Working
## Docker Development
> The following documentation describes how to work with Mermaid in a Docker
> environment. Go back to
> [Technical Requirements](#technical-requirements) if you want to
> to work in your host environment, without Docker.
### Docker Technical Requirements
### Make `./run` executable
### Install packages in Docker
### Launch in Docker
### Verify Everything is Working in Docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. In terms of supporting this - many be. But I think combining would rather distract readers. There are many similar parts inside, some phrases are repeated too. I'd rather stick to separate page instead of a long and bewildering one. I don't actually know. I am not a technical writer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmmm, I still think having everything in one file is nicer, but I'm not a technical writer either 😆.
Having everything in a single file would be much cleaner from a writing perspective, and it will make it easier to work with the documentation.
But having them in two separate files might be better for readers.
@sidharthv96, any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am the only one who develops it in Docker...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am the only one who develops it in Docker...
I'm generally using Docker too (well, podman
, but from a user's perspective, Podman is pretty similar to Docker).
Although I'm currently running all the commands manually, which works pretty well, except for opening the Cypress GUI. It looks like the ./run
script handles Cypress pretty well, so the next time I need to debug a Cypress E2E test, I'll probably read your docs on how to get it working in Docker 😄.
91c6d5b
to
e631130
Compare
Some minor fixes for code.md Prettier Update docs Empty Experiments Update docs Revert "Experiments" This reverts commit a5f24af.
8ab2b60
to
711b40b
Compare
Thanks for thinking of me, @nirname . Probably not able to get to it any time soon, though. |
@aloisklink You give me controversial instructions.
You said that it CONTRIBUTIONS.md (and that is a combinations of files)
Which one is true?
The the major part of this PR is simplifying and splitting the pages. If we decide to combine pages we simply will end up were we started, this is exactly opposite of what I am trying to achieve here. So if you think this is wrong, lets simply reject it.
upon which I dont know what to do So the whole PR became messy |
@aloisklink the plan was to merge the docs change to master, and then merge master to develop as usual. I'm not sure how that would develop conflicts? If any conflicts do arise, we can resolve them when merging master to develop. @nirname should we squash and merge this as a single commit?
My understanding was that the CONTRIBUTIONS.md is a very minimal set of instructions to get someone who is already familiar with development started on mermaid. We can have links to the detailed instructions from there. I suggest we stick to the original plan and not make any changes to CONTRIBUTIONS.md in this PR.
@nirname highlighted some points on why keeping them separate is better. Let's keep them separate. |
Converting to draft so avoid acciendtally merging to master. |
@sidharthv96 I think I'll redo it, the whole PR is unusable. Guidelines says to target develop, so let's stick to that. I'll split this PR into several parts so that it would be easier to review. I still cannot launch |
@nirname This works for me:
I know you are doing a new PR and wanted to throw in my two cents:
Based on the Mermaid JS slack workspace, it looks like there is a sprinkling of users looking to contribute (to docs and diagrams), so concise and clear instructions would be really great. I usually share something like this when someone mentions they are interested in contributing: Check out this list of good first issues. And read our docs on contributing. Some instructions on getting started: Get started:
To contribute to the documentation:
To contribute to the core code:
To add E2E Tests:
To submit a Pull Request:
|
@huynhicode Thanks for advice. Does
works in your case in development branch? |
pnpm run --filter docs docs:dev sorry I thought it was a command in |
@nirname Yes, this works for me. I am on the |
@huynhicode no, I am asking specifically about whether this command:
works when you are in the root of mermaid project and at develop branch? If this one does not work, the only point of going directly to |
This command also works for me at the I think open source contribution should be a fun experience and not being able to set up or run the project locally is a huge deterrent. I really would like to foster a robust community, so I am in favor of whatever we need to do to make contributions to the docs or to Mermaid syntax as seamless a process as possible. |
@Yokozuna59 I am going to split instruction into different files, so it's better to wait for my PR's. |
Sorry, that's my fault, I should have been clearer. My main worry was that the current I felt like making
Ah, sorry, I saw the "targeted (I swear I saw that your PR also had some commits from @Yokozuna59 too, so I assumed those were commits from the The whole merging to As an example, if @Yokozuna59 had edited the same files on the Of course if @sidharthv96 says it's okay, Sid should know what's in both 👍 on switching this to smaller PRs! I know making smaller PRs is a lot more work, but it does making reviewing a lot easier; a big PR like this might take 1-2 hours to review, so it can be a struggle for me to find time to review a big PR (it often needs to wait until the weekend), so smaller PRs are super appreciated by me 😄! And even when I'm using other third-party libraries, smaller PRs make it a lot easier to figure out what has changed between releases, so I'm sure Mermaid users will appreciate them too!
I agree with everything else @huynhicode says in #4596 (comment), but I tend to use the "Edit this page on GitHub" link all the time (both in the Mermaid project and other projects), in order to find out which documentation file I need to edit, so please don't remove it! But I wouldn't mind changing it to "View the source for this file on GitHub", because I almost never use GitHub's UI to edit anything. |
@aloisklink No worries, the whole PR was a sloppy one, because it addressed to many different problems at once. We will:
So yes, there will be one page version for the reader to refer to. Then we will bring:
as close as possible to documentation pages Also:
Plus some other changes. I think I will add my ideas to #2910 I will target |
📑 Summary
When I started contributing, I realized that contribution part of the documentation is far from perfect. The most important commands to start development are missing or scattered across multiple documents.
Revise documentation #2910 #3416 #3744
📏 What has been done and why
n00b-overview page
and the page itself📋 Tasks
Make sure you
develop
branch