Add quality guidelines page #219

lisa-wolfgang · 2022-07-18T23:27:40Z

This PR adds quality guidelines for PRs to the main extension repo.

Since the addon compatibility checklist will be a large undertaking, it will be PRed separately.

Samq64 · 2022-07-19T01:17:08Z

I think creating-an-addon.md needs to be re-done, but I'm not sure if that should go in this PR.

lisa-wolfgang · 2022-07-19T01:20:45Z

This PR should be for new docs content related to the overhauled PR merge system in ScratchAddons/ScratchAddons.

- Navbar - Editor (general) - Stage header

lisa-wolfgang · 2022-07-19T22:47:31Z

The addon compatibility list will be a relatively large undertaking. Contributions from those experienced in specific areas of the website and editor would be greatly appreciated. See #225

It will get its own PR

lisa-wolfgang · 2022-08-02T20:07:37Z

It might be a good idea to require approvals to be accompanied by a checklist (as a comment on the approval) that we provide on this page. That way, the person approving the PR communicates exactly what they've looked at.

content/docs/reference/quality-guidelines.md

lisa-wolfgang · 2022-10-02T21:49:02Z

There hasn't been any additional feedback on these guidelines for a while, so I'm going to assume that they're final for the time being. However, to have any actual bearing on the PR approval process, org members will need to become familiar (and agree) with these guidelines.

@WorldLanguages @apple502j @GarboMuffin @GrahamSH-LLK @Hans5958 @jeffalo @mxmou @RedGuy12 @TheColaber @Weredime Please either mark your approval of these guidelines (by approving the PR) or leave a review requesting changes.

cobaltt7 · 2022-10-03T14:21:22Z

content/docs/reference/quality-guidelines.md

+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.
+- 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.
+- 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.**


What if you don't have access to the other browser yourself? What should the PR author or reviewers do in that case?

Perhaps testing on both browsers could be a requirement for approving a PR but not for opening one.

What I meant on this point was what Maximouse said.

Reviewers can still give good feedback if they don't have access to both browsers, but full approval should require a thorough evaluation, and that can't happen if one of our supported browsers hasn't been tested.

alr so i'm never approving prs again
got it 👍🏻

Agreed on making it a recommendation.

I think it can be helpful sometimes. For complex changes or changes to important parts of Scratch Addons like the settings page, I think we should request testing on Firefox before merging, but right now, we may not need to do that for everything.

cobaltt7 · 2022-10-03T14:22:44Z

content/docs/reference/quality-guidelines.md

+- 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.
+
+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.


Specify American English?

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

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

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.

cobaltt7 · 2022-10-03T14:23:24Z

content/docs/reference/quality-guidelines.md

+- 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.
+- 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 work without any other addons enabled.


Suggested change

- must work without any other addons enabled.

- must also work without any other addons enabled.

to avoid confusion with the line above

Adding "also" seems okay to me

cobaltt7 · 2022-10-03T14:25:04Z

content/docs/reference/quality-guidelines.md

+<!-- 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


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.

~~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)

content/docs/reference/quality-guidelines.md

cobaltt7 · 2022-10-03T14:28:05Z

content/docs/reference/quality-guidelines.md

+[] 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
+[] I personally tested with no other addons enabled and did not find issues
+[] The addon is well-integrated with other addons


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

Maybe that could be clarified with "if appropriate".

cobaltt7 · 2022-10-03T14:31:49Z

It may be a good idea to ask contributors to join the Discord server or at least create an issue for discussion about PRs before creating them -- often times people try to make rejected or very complex addons as their first PR.

content/docs/reference/quality-guidelines.md

Hans5958 · 2022-10-07T17:04:39Z

It may be a good idea to ask contributors to join the Discord server or at least create an issue for discussion about PRs before creating them -- often times people try to make rejected or very complex addons as their first PR.

I'd rather have a focus to create an issue compared to joining the Discord server.

Hans5958 · 2022-10-19T13:04:27Z

content/docs/reference/quality-guidelines.md

@@ -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.
 - 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.


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.

WorldLanguages · 2022-12-26T22:33:43Z

We should try to get this merged sometime soon, even if it's not 100% perfect. We can always tweak it later.

content/docs/reference/quality-guidelines.md

Hans5958

Putting my early approval here. Changes may still made, but I won't merge it.

DNin01 · 2023-03-19T21:46:08Z

content/docs/reference/quality-guidelines.md

Maybe mention not affecting non-SA users here?

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

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

Hans5958 · 2023-05-01T07:00:29Z

Bump to have fresh ideas

Samq64 · 2023-05-01T13:10:43Z

https://scratchaddons.com/docs/develop/userscripts/best-practices/ exists now and there will probably be a similar page for userstyles in the future, so we should probably link those.

DNin01 · 2023-07-03T19:49:24Z

content/docs/reference/quality-guidelines.md

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.

Samq64 · 2023-07-06T15:45:27Z

content/docs/reference/quality-guidelines.md

+- 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.
+- 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.
+- 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.**


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.

Add quality guidelines, mention in addon tutorial

e040b3e

lisa-wolfgang mentioned this pull request Jul 18, 2022

[Pending docs PR] Update CONTRIBUTING.md ScratchAddons/ScratchAddons#4855

Closed

lisa-wolfgang added 2 commits July 18, 2022 19:25

Merge branch 'master' into pr-standards

4069dcd

Change addon compat page name

0be88ba

lisa-wolfgang added 2 commits July 18, 2022 22:13

Compat list additions

d63e182

- Navbar - Editor (general) - Stage header

Compat list: fix inaccuracies in Stage Header

271fda2

lisa-wolfgang added help wanted Extra attention is needed scope: docs Related to the documentation (Scratch Addons Docs) labels Jul 19, 2022

Hans5958 added the type: enhancement New feature or request label Jul 20, 2022

lisa-wolfgang added 2 commits July 20, 2022 17:42

Quality guidelines: make testing required

f6765d5

Remove addon compat list

9f5949b

It will get its own PR

lisa-wolfgang changed the title ~~Add quality guidelines and addon compatibility checklist~~ Add quality guidelines page Jul 21, 2022

lisa-wolfgang removed the help wanted Extra attention is needed label Jul 21, 2022

Move to reference category

05dc38e

lisa-wolfgang marked this pull request as ready for review July 21, 2022 02:02

lisa-wolfgang requested review from WorldLanguages and apple502j July 21, 2022 02:03

Add approval checklist

5dbd3b7

mxmou reviewed Aug 5, 2022

View reviewed changes

content/docs/reference/quality-guidelines.md Show resolved Hide resolved

Add standalone addon requirement

8de282c

lisa-wolfgang requested review from cobaltt7, apple502j, GarboMuffin and GrahamSH-LLK and removed request for WorldLanguages and apple502j October 2, 2022 21:48

lisa-wolfgang requested review from Hans5958, jeffalo, mxmou, TheColaber, Weredime and WorldLanguages October 2, 2022 21:48

Merge remote-tracking branch 'upstream/master' into pr-standards

1c6992a

cobaltt7 suggested changes Oct 3, 2022

View reviewed changes

mxmou reviewed Oct 3, 2022

View reviewed changes

content/docs/reference/quality-guidelines.md Show resolved Hide resolved

RTL

3b3c669

Hans5958 reviewed Oct 19, 2022

View reviewed changes

DNin01 reviewed Jan 20, 2023

View reviewed changes

content/docs/reference/quality-guidelines.md Show resolved Hide resolved

Hans5958 approved these changes Jan 26, 2023

View reviewed changes

Hans5958 force-pushed the master branch from da240b9 to 25a97f1 Compare March 12, 2023 08:03

DNin01 reviewed Mar 19, 2023

View reviewed changes

DNin01 reviewed Jul 3, 2023

View reviewed changes

Samq64 reviewed Jul 6, 2023

View reviewed changes

	- must work without any other addons enabled.
	- must also work without any other addons enabled.

Add quality guidelines page #219

Are you sure you want to change the base?

Add quality guidelines page #219

Conversation

lisa-wolfgang commented Jul 18, 2022 • edited Loading

Samq64 commented Jul 19, 2022 • edited Loading

lisa-wolfgang commented Jul 19, 2022

lisa-wolfgang commented Jul 19, 2022 • edited Loading

lisa-wolfgang commented Aug 2, 2022 • edited Loading

lisa-wolfgang commented Oct 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DNin01 Jan 20, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hans5958 Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cobaltt7 commented Oct 3, 2022

Hans5958 commented Oct 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldLanguages commented Dec 26, 2022

Hans5958 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WorldLanguages Mar 19, 2023 • edited by DNin01 Loading

Choose a reason for hiding this comment

WorldLanguages Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

Hans5958 commented May 1, 2023

Samq64 commented May 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisa-wolfgang commented Jul 18, 2022 •

edited

Loading

Samq64 commented Jul 19, 2022 •

edited

Loading

lisa-wolfgang commented Jul 19, 2022 •

edited

Loading

lisa-wolfgang commented Aug 2, 2022 •

edited

Loading

DNin01 Jan 20, 2023 •

edited

Loading

Hans5958 Oct 19, 2022 •

edited

Loading

WorldLanguages Mar 19, 2023 •

edited by DNin01

Loading

WorldLanguages Mar 21, 2023 •

edited

Loading