⚒️ Set acceptance criteria for pull requests

[tatianamac]

Currently pull requests do not have written criteria around them.

Setting some written criteria would help to eliminate PRs that aren't quite ready AND to provide clearer guidance for new contributors (and new-to-OSS contributors).

A few non-exhaustive notes:

• Editorial guidelines
• Linting
• Commit notes

[ovlb]

I can formalise the things I look for before approving a PR over the next couple days.

Where do you envision these guidelines to live?

[tatianamac]

I can formalise the things I look for before approving a PR over the next couple days.

That'd be helpful if you could backdocument those here—thanks!

Where do you envision these guidelines to live?

Documentation, which is going to be my focus for the next few weeks. In addition to the technical aspects, I'd like to reduce barriers to entry while making more clear what editorial content will be accepted and what will be rejected and on what premise.

[kkemple]

I wonder if issue and PR templates could help with this as well!

[tatianamac]

@kkemple Yes, good idea! I had started issue templates awhile back and forgotten!

[tatianamac]

Re: pull request acceptance:

• Minimum two reviewers (exceptions noted below)
• For definitions: If not written by someone who identifies as or is most impacted by the term, then one reviewer must and act as a sensitivity reader

Exceptions

• Typos
• Broken links

[ovlb]

This is a first batch of things I tend to look out for when reviewing code-centric PRs. Quite a bit, actually. Might rewrite/add to this list in the upcoming days. So consider it a working draft. :)

• Does the feature work/is the bug fixed? This might be obvious, but especially a more complex feature might need more than one dev/review cycle. So not every review needs to end in acceptance. And it’s also totally valid to ask for a review mid-development to see if you’re on the right track.
• Is the implementation accessible? The specific steps here depend heavily on the feature under development. For larger features, I tend to use at least two browser/screen reader combinations before accepting.
• Is the proposed solution understandable? Sometimes code that’s not abstract is more understandable than a super clever one-liner. When in doubt, I tend to prefer code that does not need a computer science major to understand. Another example here is templating logic which is very complex (e.g. meta descriptions, or flags). Instead of writing lengthy template logic which is hard to understand and impossible to test, this code should live as part of shortcode/filter and have unit tests.
• Are code additions/modifications are in the right place? As I made up lots of the codebase, I might know of a similar bit of code, especially folx just getting to know the codebase might not have this knowledge.
• Code quality. This is a heavily subjective topic. I know. Things like naming, comments where things get weird etc. count into it.
  • HTML: Are the appropriate elements used, is the structure valid, this kind of things
  • CSS: Is the implementation using modern CSS where applicable, is the CSS backwards-compatible, and, yes, does it work in IE11
  • JavaScript: We use linting and Prettier anyway, so this does not mean nitty-gritty but-the-tabs-should-be-spaces kind of stuff. Instead, I’m looking, as an example, at the usage of Node/Browser APIs and if an implementation might be simplified by using them.
• Is there anything left to do? Sometimes there are things that come up while developing a feature which would broaden the scope of the initial PR so much that a follow-up is needed. While this is no blocker to accept and merge a PR, it’s still on my radar so that nothing falls behind.

O*

[UdokaVrede]

I would make a pr on this

[kkemple]

This will be resolved in #455 which adds a PR template. Definitely open to feedback on the structure and content of the template!