-
Notifications
You must be signed in to change notification settings - Fork 2
41 contribution guidestandards for new contributors #266
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| ## Summary | ||
|
|
||
| - Describe the change. | ||
| - Link the related issue if there is one. | ||
|
|
||
| ## Checklist | ||
|
|
||
| - [ ] I ran `npm run lint` | ||
| - [ ] I ran `npm test` or documented why tests were not needed | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to linting, no tests currently exist outside of a few for gw-merge, so a contributor is going to have a hard time determining what the expectation is. Unit tests with RTL/Vitest, accessibility tests, etc.? There's not really a baseline to work from. |
||
| - [ ] I updated docs/examples if the component API or behavior changed | ||
| - [ ] I added screenshots for UI changes when useful | ||
| - [ ] I applied exactly one version label: `patch-bump`, `minor-bump`, or `major-bump` | ||
|
|
||
| ## Release impact | ||
|
|
||
| - Explain why the selected version bump label is correct. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # Contributing to Groundwork | ||
|
|
||
| Groundwork is a shared React component library for USACE applications. Keep changes small, well-scoped, and documented so maintainers can review and release them predictably. | ||
|
|
||
| ## Development setup | ||
|
|
||
| Groundwork's GitHub Actions workflows run on Node 20. Use the same version locally when possible. | ||
|
|
||
| ```bash | ||
| npm install | ||
| npm run dev | ||
| npm test | ||
| npm run lint | ||
| ``` | ||
|
|
||
| ## Coding standards | ||
|
|
||
| - Use Prettier formatting. A Husky pre-commit hook runs `lint-staged`, which applies Prettier to staged `js`, `jsx`, `json`, `css`, and `md` files. | ||
| - Run `npm run lint` before opening a pull request. ESLint is configured for the React codebase and should stay clean. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as in PR template -- I think we either need to clean up the repo or clarify expectations. Is the first person to contribute expected to clean up all of the existing issues? |
||
| - Add or update tests when behavior changes. `lib/gw-merge.test.js` shows the existing Vitest setup. | ||
| - Update the documentation app in `src/app-pages/documentation` when a component API, behavior, or usage pattern changes. | ||
| - Edit source files in `src` and `lib`. Do not hand-edit generated output in `dist` or the built docs output in `docs`. | ||
|
|
||
| ## Branch naming | ||
|
|
||
| Branch from issue so everything is connected. This means every change starts with an issue rather than just an "idea". | ||
|
|
||
| ## Pull requests | ||
|
|
||
| - Link the issue or describe the problem the PR solves (See Branch Naming above) | ||
| - Keep each PR focused on one change set. Separate refactors from behavior changes when possible. | ||
| - Include screenshots or short screen recordings for visible UI changes. | ||
| - Call out any accessibility, API, or migration impact in the PR description. | ||
| - Request review from a Groundwork maintainer before merging. | ||
|
|
||
| ## Reviews | ||
|
|
||
| Reviewers should be able to answer three questions quickly: | ||
|
|
||
| 1. Is the change scoped correctly for Groundwork? | ||
| 2. Are tests and docs updated for the new behavior? | ||
| 3. Is the selected version bump correct? (Major, Minor, Patch) | ||
|
|
||
| Contributors should address review comments with follow-up commits unless a maintainer asks for a different workflow. | ||
|
|
||
| ## Versioning and releases | ||
|
|
||
| Groundwork uses semantic versioning and enforces version bump labels on pull requests to `main`. | ||
|
|
||
| - `patch-bump`: bug fixes, docs-only changes, maintenance work, and backward-compatible internal cleanup | ||
| - `minor-bump`: new backward-compatible components, props, or behavior | ||
| - `major-bump`: breaking API changes, removals, or behavior that requires consumer updates | ||
|
|
||
| Current repository automation expects exactly one of these labels on every PR to `main`: | ||
|
|
||
| - `patch-bump` | ||
| - `minor-bump` | ||
| - `major-bump` | ||
|
|
||
| After a PR is merged into `main`, the `pr-close.yml` workflow bumps the package version and pushes the tag. Publishing to npm happens when a GitHub Release is published, which triggers `.github/workflows/publish.yml`. | ||
|
|
||
| ## Commit guidance | ||
|
|
||
| - Prefer small commits with clear messages that describe the intent of the change. | ||
| - Avoid mixing formatting-only changes with feature or bug-fix commits unless the formatter was required for touched files. | ||
| - Do not rewrite shared branch history unless the maintainers ask for it. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a tangible benefit to having the contribution guide included in the library docs as opposed to just linking to CONTRIBUTING.md? As usual, I worry a bit about having multiple sources of truth. There's already a little discrepancy in the language related to linting. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| import { Code, H3, Text, TextLink, UsaceBox } from "../../../../lib"; | ||
| import DocsPage from "../_docs-page"; | ||
|
|
||
| const pageBreadcrumbs = [ | ||
| { | ||
| text: "Documentation", | ||
| href: "/#/docs", | ||
| }, | ||
| { | ||
| text: "Getting Started", | ||
| href: "/#/docs", | ||
| }, | ||
| { | ||
| text: "Contribution Guide", | ||
| href: "/#/docs/contribution-guide", | ||
| }, | ||
| ]; | ||
|
|
||
| function P({ children }) { | ||
| return <Text className="gw-pt-3 gw-pb-3">{children}</Text>; | ||
| } | ||
|
|
||
| function ContributionGuide() { | ||
| return ( | ||
| <DocsPage breadcrumbs={pageBreadcrumbs}> | ||
| <UsaceBox title="Contribution Guide"> | ||
| <div className="gw-pb-6"> | ||
| <P> | ||
| Groundwork is a shared React component library for USACE | ||
| applications. Contributions should stay focused, keep the component | ||
| library stable for downstream consumers, and include the docs or | ||
| tests needed to support the change. | ||
| </P> | ||
|
|
||
| <H3 className="gw-mt-6">Development setup</H3> | ||
| <P> | ||
| Groundwork's GitHub Actions workflows run on Node 20, so use | ||
| the same version locally when possible. | ||
| </P> | ||
| <Code className="!gw-font-bold">terminal</Code> | ||
| <pre className="gw-mt-3 gw-overflow-x-auto gw-rounded gw-bg-slate-900 gw-p-4 gw-text-sm gw-text-white"> | ||
| <code>{`npm install | ||
| npm run dev | ||
| npm test | ||
| npm run lint`}</code> | ||
| </pre> | ||
|
|
||
| <H3 className="gw-mt-6">Coding standards</H3> | ||
| <ul className="gw-list-disc gw-pl-6 gw-pt-3 gw-pb-3"> | ||
| <li> | ||
| Use Prettier formatting. A Husky pre-commit hook runs{" "} | ||
| <Code>lint-staged</Code> on staged <Code>js</Code>,{" "} | ||
| <Code>jsx</Code>, <Code>json</Code>, <Code>css</Code>, and{" "} | ||
| <Code>md</Code> files. | ||
| </li> | ||
| <li> | ||
| Run <Code>npm run lint</Code> before opening a pull request. | ||
| </li> | ||
| <li> | ||
| Add or update tests when behavior changes. Groundwork uses Vitest | ||
| for the current test suite. | ||
| </li> | ||
| <li> | ||
| Update the documentation pages when a component API, usage | ||
| pattern, or behavior changes. | ||
| </li> | ||
| <li> | ||
| Edit source in <Code>src</Code> and <Code>lib</Code>. Do not | ||
| hand-edit built output in <Code>dist</Code> or <Code>docs</Code>. | ||
| These are generated from the source files and will be overwritten | ||
| by the build process. | ||
| </li> | ||
| </ul> | ||
|
|
||
| <H3 className="gw-mt-6">Routing links in components</H3> | ||
| <P> | ||
| When adding links inside Groundwork components, prefer the exported{" "} | ||
| <Code>{`<Link />`}</Code> component instead of hard-coding raw{" "} | ||
| <Code>{`<a>`}</Code> elements where LinkProvider support is | ||
| expected. That allows applications using{" "} | ||
| <Code>{`<LinkProvider />`}</Code> to swap in their routing | ||
| library's link component correctly. | ||
| </P> | ||
| <P> | ||
| See the{" "} | ||
| <TextLink href="/#/docs/navigation/link-provider"> | ||
| Link Provider documentation | ||
| </TextLink>{" "} | ||
| for the routing integration details and expected behavior. | ||
| </P> | ||
|
|
||
| <H3 className="gw-mt-6">Branch naming</H3> | ||
| <P> | ||
| Use short-lived topic branches for each change. Prefer patterns like{" "} | ||
| <Code>feature/issue-<number>-short-description</Code>,{" "} | ||
| <Code>fix/issue-<number>-short-description</Code>,{" "} | ||
| <Code>docs/issue-<number>-short-description</Code>, or{" "} | ||
| <Code>chore/issue-<number>-short-description</Code>. | ||
| </P> | ||
| <P> | ||
| If there is no issue number yet, omit that segment and keep the | ||
| description concise. | ||
| </P> | ||
|
|
||
| <H3 className="gw-mt-6">Pull requests and reviews</H3> | ||
| <ul className="gw-list-disc gw-pl-6 gw-pt-3 gw-pb-3"> | ||
| <li> | ||
| Link the issue or describe the problem the pull request solves. | ||
| </li> | ||
| <li>Keep each pull request focused on one change set.</li> | ||
| <li>Include screenshots for visible UI changes when useful.</li> | ||
| <li> | ||
| Call out accessibility, API, or migration impact in the pull | ||
| request description. | ||
| </li> | ||
| <li>Request review from a Groundwork maintainer before merging.</li> | ||
| </ul> | ||
| <P> | ||
| Reviewers should be able to confirm the scope is appropriate, the | ||
| docs and tests match the change, and the selected version bump is | ||
| correct. | ||
| </P> | ||
|
|
||
| <H3 className="gw-mt-6">Versioning and releases</H3> | ||
| <P> | ||
| Groundwork uses semantic versioning and requires exactly one version | ||
| label on every pull request to <Code>main</Code>. | ||
| </P> | ||
| <ul className="gw-list-disc gw-pl-6 gw-pt-3 gw-pb-3"> | ||
| <li> | ||
| <Code>patch-bump</Code>: bug fixes, docs-only changes, and | ||
| backward-compatible maintenance work | ||
| </li> | ||
| <li> | ||
| <Code>minor-bump</Code>: new backward-compatible components, | ||
| props, or behavior | ||
| </li> | ||
| <li> | ||
| <Code>major-bump</Code>: breaking API changes, removals, or | ||
| behavior that requires consumer updates | ||
| </li> | ||
| </ul> | ||
| <P> | ||
| After a pull request is merged, repository automation bumps the | ||
| package version and pushes the tag. Publishing to npm happens when a | ||
| GitHub Release is published. | ||
| </P> | ||
|
|
||
| <H3 className="gw-mt-6">Reference</H3> | ||
| <P> | ||
| The canonical repo guidance also lives in{" "} | ||
| <TextLink | ||
| href="https://github.com/USACE/groundwork/blob/main/CONTRIBUTING.md" | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| > | ||
| CONTRIBUTING.md on GitHub | ||
| </TextLink> | ||
| . | ||
| </P> | ||
| </div> | ||
| </UsaceBox> | ||
| </DocsPage> | ||
| ); | ||
| } | ||
|
|
||
| export default ContributionGuide; | ||
| export { ContributionGuide }; |
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 feels a little misleading when there are currently 24 errors detected by
npm run lint. Feels like either the repo should be cleaned up first or some clarification should be added on what the expectations are.