Skip to content

docs: update contributing documentation and PR template #1411

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 16, 2022
Merged

docs: update contributing documentation and PR template #1411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
389c483
Refine the PR checklist
jzempel Sep 16, 2022
95e5cc6
Add clarifying text for code contribution
jzempel Sep 16, 2022
f83cb10
Remove product review
jzempel Sep 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,12 @@ commands are available:
template.

See Garden's [development documentation](/docs/development.md) for package
implementation requirements.
implementation requirements. In general, your code will be expected to follow
documented standards and prevailing conventions. If you feel the need to
introduce a new or novel pattern, please start a new
[discussion](https://github.com/zendeskgarden/react-components/discussions) or
open an [issue](https://github.com/zendeskgarden/react-components/issues) for
consideration by the core team.

## Pull Request Workflow

Expand Down
10 changes: 6 additions & 4 deletions .github/PULL_REQUEST_TEMPLATE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,13 @@

## Checklist

- [ ] :ok_hand: design updates are Garden Designer approved (add the
designer as a reviewer)
<!-- check the items below that will be completed prior to merge.
strikethrough any item text that does not apply to this PR. -->

- [ ] :ok_hand: design updates will be Garden Designer approved (add the designer as a reviewer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the switch to future tense?

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 want the PR author to check these items when the PR is submitted for review (aka, "I've thought of the items needed to open a PR"). We've seen folks confused about holding off on checkmarks because they haven't secured design review.

- [ ] :globe_with_meridians: demo is up-to-date (`yarn start`)
- [ ] :arrow_left: renders as expected with reversed (RTL) direction
- [ ] :metal: renders as expected with Bedrock CSS (`?bedrock`)
- [ ] :wheelchair: analyzed via [axe](https://www.deque.com/axe/) and evaluated using VoiceOver
- [ ] :guardsman: includes new unit tests
- [ ] :memo: tested in Chrome, Firefox, Safari, Edge, and IE11
- [ ] :wheelchair: tested for [WCAG 2.1](https://www.w3.org/TR/WCAG21) AA compliance
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify browser-AT combinations expected to work at minimum, or is the expectation that we would test all combinations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's discuss offline.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if it is worth noting here (or somewhere else so we don't make this list hard to read, maybe an asterisk?) that as a general rule, we stay away from adding one-off workarounds to account for browser/AT bugs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that could be an excellent addition to thedevelopment.md.

- [ ] :memo: tested in Chrome, Firefox, Safari, and Edge
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify versions (like "latest-2" or something?)

Copy link
Member Author

Choose a reason for hiding this comment

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

The target is specified. Maybe we should link? (Although I'm not super keen about cluttering the PR template with raw markdown links.)