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.

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

Jump to bottom

Add quality guidelines page #219

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add quality guidelines page #219

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e040b3e
Add quality guidelines, mention in addon tutorial
lisa-wolfgang Jul 18, 2022
4069dcd
Merge branch 'master' into pr-standards
lisa-wolfgang Jul 19, 2022
0be88ba
Change addon compat page name
lisa-wolfgang Jul 19, 2022
d63e182
Compat list additions
lisa-wolfgang Jul 19, 2022
271fda2
Compat list: fix inaccuracies in Stage Header
lisa-wolfgang Jul 19, 2022
f6765d5
Quality guidelines: make testing required
lisa-wolfgang Jul 20, 2022
9f5949b
Remove addon compat list
lisa-wolfgang Jul 21, 2022
05dc38e
Move to reference category
lisa-wolfgang Jul 21, 2022
5dbd3b7
Add approval checklist
lisa-wolfgang Aug 4, 2022
8de282c
Add standalone addon requirement
lisa-wolfgang Aug 8, 2022
1c6992a
Merge remote-tracking branch 'upstream/master' into pr-standards
lisa-wolfgang Oct 3, 2022
3b3c669
RTL
lisa-wolfgang Oct 15, 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
3 changes: 2 additions & 1 deletion content/docs/reference/quality-guidelines.md
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention not affecting non-SA users here?

Copy link
Member

@WorldLanguages WorldLanguages Mar 19, 2023 •
edited by DNin01
Loading

Choose a reason for hiding this comment

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

Related: ScratchAddons/ScratchAddons#5830

I'm not sure if a list of reasons to reject an addon belongs here. Would need to think about it

Copy link
Member

@WorldLanguages WorldLanguages Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Update: ScratchAddons/ScratchAddons#5830 (comment)
I believe the list of reasons can become its own doc page. We could add a check/mention here that links to that page, after we create the page

This comment was marked as outdated.

Sign in to view

Copy link
Member

Choose a reason for hiding this comment

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

I'm glad that we have reviews from people like mxmou that are so thorough. I know, no one likes having PRs held back due to reviews, but having stricter guidelines for reviewers to follow would make thorough reviews more valuable and could result in more problems being fixed.

Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Keep these guidelines in mind when creating or reviewing a pull request (PR). If
All PRs:
- should have sound reasoning as to why they should be merged. This should be specified under the "Reason for changes" header when writing the PR description. Even if it seems obvious, others may need the additional context. PRs with little to no reason to be merged are likely to be delayed, ignored, or even closed.
cobaltt7 marked this conversation as resolved.
Show resolved Hide resolved
DNin01 marked this conversation as resolved.
Show resolved Hide resolved
- must support both the Chromium and Firefox browser engines. Avoid using new or experimental web technologies unless a fallback is implemented that preserves feature parity. View the [unsupported browser](https://scratchaddons.com/unsupported-browser/) page to determine the current minimum versions that must be supported.
Copy link
Member

Choose a reason for hiding this comment

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

View the unsupported browser page to determine the current minimum versions that must be supported.

I'd rather link https://scratchaddons.com/docs/getting-started/installing/ instead of that page.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

- must be tested thoroughly. We have a very large userbase, so it's especially important that all possible use cases are considered. Every PR should be tested on a Chromium-based browser (Chrome, Edge, Brave, or another) **and** Firefox. Any tests done should be added to the "Tests" section of the PR description. **Org members, this is required in order to give approval.**
- must be tested thoroughly. We have a very large userbase, so it's especially important that all possible use cases are considered. Every PR should be tested on a Chromium-based browser (Chrome, Edge, Brave, or another) **and** Firefox. In addition, both LTR and RTL language modes should be supported. Any tests done should be added to the "Tests" section of the PR description. **Org members, this is required in order to give approval.**
- must have **all** code reviewed line-by-line. **Org members, this is required in order to give approval.**
Copy link
Member

Choose a reason for hiding this comment

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

I know this would be ideal, but larger PRs already sometimes sit there for months because no org member wants to tackle reviewing a few thousand lines of code. I know large PRs can usually be split, but it's not always possible.

- should be reviewed by someone familiar with the aspects that the PR is changing. This isn't a hard requirement, but a person like this will help iron out edge cases and minor issues.

Expand All @@ -24,6 +24,7 @@ In order for an approval to be valid, it must have the following checklist accom
[] Valid reason to merge
[] To the best of my knowledge, supports all browsers supported by the extension
[] I personally tested all features in both a Chromium-based browser and Firefox
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto from above
Many people, including me, do not have access to another browser for any number of reasons. if we make it a requirement to test on both, I know I for one will not be making or reviewing any more PRs, and I'm sure many others won't either.

Copy link
Member

@Hans5958 Hans5958 Oct 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'd rather have this as a recommendation, and not a strict requirement. 90% of the case it will work on both browsers, so it is not something of a priority. At least that's on me.

See message on #219 (comment)

[] All added UI elements support both LTR and RTL language modes
[] I personally reviewed all code line-by-line
<!-- For new addons and features, the following checklist items must also be included and completed. -->
[] All titles, descriptions, info notices, and setting names provided are clear and concise
Expand Down