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
19 changes: 18 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 @@ -15,4 +15,21 @@ All PRs:
New addons:
- must have clear, concise, and easy-to-understand titles, descriptions, info notices (if any), and setting names (if any). English is required: the base addon manifest is used as a reference for our translators. The developers and members of the community should scrutinize and help with this. An effective meter for this is to test it **blindly** -- that is, download and try out the addon before reading the PR description.
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify American English?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, IIRC we have a page somewhere for often used names of Scratch pages and features, can we link that here?

Copy link
Member

Choose a reason for hiding this comment

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

We do: https://scratchaddons.com/docs/reference/addon-manifest/#description

Copy link
Member

Choose a reason for hiding this comment

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

IMO, we should have a dedicated page about this, instead of linking to the "description" section of the addon manifest reference. In the meanwhile however, we could link to that.

- must be easy to use. The labels and design language used in any UI that the addon adds or modifies should be understandable by whoever will be using that addon. Again, testing blindly is an effective meter for this. Addons without an effective UI communicating its changes will be ranked lower than other addons in the settings page.
- must be compatible with our existing addons. This doesn't just mean avoiding bugs: addons should complement each other by supporting each other's features. For example, an addon that has a user interface should support our respective dark mode addon.
- must be compatible with our existing addons. This doesn't just mean avoiding bugs: addons should complement each other by supporting each other's features. For example, an addon that has a user interface should support our respective dark mode addon.

In order for an approval to be valid, it must have the following checklist accompanying it as a comment:
```markdown
<!-- The following checklist items apply to all PRs. -->
[] 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)

[] 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
[] I personally found the addon to be easy to use during my testing
[] I personally tested with all other addons and features enabled and did not find compatibility issues
mxmou marked this conversation as resolved.
Show resolved Hide resolved
[] The addon is well-integrated with other addons
Copy link
Contributor

Choose a reason for hiding this comment

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

Most addons don't need special changes when other addons are toggled...I think the line two above covers this.

Suggested change
[] The addon is well-integrated with other addons

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe that could be clarified with "if appropriate".

<!-- The following checklist items aren't required, but please check them if applicable to give us as much info on your approval as possible. -->
[] I blind-tested this PR
cobaltt7 marked this conversation as resolved.
Show resolved Hide resolved
[] I'm a specialist in the area this PR is changing
```