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

WIP update the CONTRIBUTING.md guide. #1203

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

Conversation

trusktr
Copy link
Member

@trusktr trusktr commented Jun 5, 2020

Summary

Improve the contributing guidelines and documentation.

TODO:

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI of default theme, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)

You have tested in the following browsers: (Providing a detailed version will be better.)

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature
  • Related documents have been updated
  • Related tests have been updated

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

Just updating docs for contributing.

@vercel
Copy link

vercel bot commented Jun 5, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/rf0rv0cud
✅ Preview: https://docsify-preview-git-contributing-guide.docsify-core.vercel.app

```sh
npm install
npm run build
npx serve .
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should mention the dev command before this ?

like after install
npm run dev for streamline one you mentioned below and then this build thing ?

WDYT ?

cause we have the same in gitpod environment as well so for consistency we can have that.

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 should suggest using npx when we have npm scripts setup to accomplish the same thing (in this case, npm run serve). npx requires internet access which means it will fail when working offline, can be slow when internet speeds are slow (rural areas, mobile, developing countries, etc.) and/or expensive when internet charges are high (mobile, bandwidth limits, etc.)

Let's make sure the repo has everything needed for local development.

@trusktr
Copy link
Member Author

trusktr commented Jun 6, 2020 via email

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 8, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 34df471:

Sandbox Source
docsify-template Configuration

@trusktr trusktr added do not merge WIP docs related to the documentation of docsify itself website related to the official docsify website: https://docsify.js.org and removed do not merge labels Jun 19, 2020
@trusktr trusktr mentioned this pull request Jul 7, 2020
15 tasks
@kirbyfern kirbyfern mentioned this pull request Jul 8, 2020
15 tasks
Copy link
Member

@jhildenbiddle jhildenbiddle left a comment

Choose a reason for hiding this comment

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

Per the #maintainers discussion on Discuss, let's also add information regarding our agreed commit convention: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit

```sh
npm install
npm run build
npx serve .
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 should suggest using npx when we have npm scripts setup to accomplish the same thing (in this case, npm run serve). npx requires internet access which means it will fail when working offline, can be slow when internet speeds are slow (rural areas, mobile, developing countries, etc.) and/or expensive when internet charges are high (mobile, bandwidth limits, etc.)

Let's make sure the repo has everything needed for local development.

npx serve .
```

The third command will output a URL like `http://localhost:5000`. Open it in your browser, then you'll see a directory listing. Click on the `docs/` folder and you'll then see the Docsify documentation (built with Docsify of course!) running locally on your computer.
Copy link
Member

Choose a reason for hiding this comment

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

Development server runs on port 3000.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • update port
  • change to npm command

I agree about the change. Sidenote, I think npx falls back to the same as npm and uses the local dependency without the internet when the local dependency is installed.

@trusktr
Copy link
Member Author

trusktr commented Oct 9, 2020

Per the #maintainers discussion on Discuss, let's also add information regarding our agreed commit convention: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit

👍

  • update commit format

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs related to the documentation of docsify itself website related to the official docsify website: https://docsify.js.org WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants