-
Notifications
You must be signed in to change notification settings - Fork 39
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.
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
Add quality guidelines page #219
base: master
Are you sure you want to change the base?
Changes from all commits
e040b3e
4069dcd
0be88ba
d63e182
271fda2
f6765d5
9f5949b
05dc38e
5dbd3b7
8de282c
1c6992a
3b3c669
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,82 +1,98 @@ | ||
--- | ||
title: Creating an Addon | ||
weight: 3 | ||
aliases: | ||
- /docs/developing/getting-started/creating-an-addon | ||
--- | ||
Required software: text editor, Chrome. | ||
If possible, disable the Scratch Addons extension you downloaded from the store before proceeding to avoid issues. | ||
|
||
## Step 1: Read about [addon basics](/docs/develop/getting-started/addon-basics/) | ||
Make sure to read that article to be familiar with the terminology. | ||
|
||
## Step 2: Fork/clone the repo | ||
Or download as ZIP, if you want. In other words, just download the source code locally. | ||
|
||
## Step 3: Load the extension into Chrome | ||
*Note: Chrome is recommended for working on addons. Nevertheless, addons are expected to work on Firefox and Edge.* | ||
Now that you have the extension in your filesystem, go to `chrome://extensions` and toggle "developer mode". | ||
Click "load unpacked", then select the folder where Scratch Addons is located. If you're having issues with this, make sure to be selecting the folder where `manifest.json` is located. | ||
That's it, you loaded the extension! It should look similar to this: | ||
![image](https://user-images.githubusercontent.com/17484114/91502527-accfd580-e89e-11ea-9e16-7daa2b808379.png) | ||
Note: you can safely ignore it says "errors". That's just a warning for an unrecognized manifest key that's required by Firefox. | ||
|
||
## Step 4: What's your addon about? | ||
Now comes the fun part! | ||
What will your addon do? Think of a self descriptive addon ID (no spaces or special characters, except hyphens). | ||
Got it? | ||
|
||
## Step 5: Create the folder for the addon | ||
Using a file explorer, go to the folder where Scratch Addons resides in your filesystem. Locate the `addons` folder. | ||
Then, create a new folder with your epic addon ID as its name. | ||
|
||
## Step 6: Add an addon manifest | ||
The addon manifest tells Scratch Addons how your addon works. Make sure to get this right to avoid headaches. | ||
Inside the folder you just created, create an `addon.json` file. | ||
This is a base you can use to start coding, make sure to change it in the future: | ||
```json | ||
{ | ||
"name": "Epic placeholder text in place of addon name", | ||
"description": "Hello World! It would be really smart to replace this placeholder text with a description.", | ||
"tags": ["community"], | ||
"enabledByDefault": false | ||
} | ||
``` | ||
For more information on what you can declare in the manifest, check [this article](/docs/reference/addon-manifest/). | ||
|
||
|
||
## Step 7: Tell Scratch Addons what your addon's ID is | ||
Scratch Addons can't find new folders by itself, so you have to add the name to a special file. | ||
Go to `scratchAddonsFolder/addons/addons.json` and add the ID of your addon to the array. | ||
|
||
## Step 8: Hello world | ||
Your addon does nothing at the moment, so it's a good time to check if everything we made previously worked. | ||
Go to `chrome://extensions` and reload Scratch Addons by clicking the refresh symbol on its card. | ||
Now, right-click the Scratch Addons icon, and click "options". | ||
You should see your addon on the list! Once you find it, enable it, and set any settings that you may have. | ||
|
||
## Step 9: The fun part, code! | ||
*Before proceeding, make sure you read the wiki article linked in step 1.* | ||
|
||
Here comes the fun part: create your own JS or CSS files! | ||
Protip: after making any change to your addon, make sure to refresh the Scratch Addons extension like you did in step 8. | ||
|
||
Depending on what you want your addon to do, you should now check these wiki pages: | ||
- [Userscripts](/docs/develop/addon-types/userscripts) | ||
- [Userstyles](/docs/develop/addon-types/userstyles) | ||
|
||
## Step 10: Make your addon customizable | ||
If you want, you can make your addon customizable! | ||
Users of your addon will be able to toggle settings, enter numbers, and more! | ||
To get started, see [how to declare settings in the addon manifest](/docs/reference/addon-manifest/#settings-object). | ||
Then, head to the [addon.settings documentation](/docs/reference/addon-api/addon.settings) to learn how to access user choices from userscripts. | ||
|
||
## Step 11: Before publishing your addon | ||
Now that your addon works, let's make sure we can add it to the addon library. | ||
Make sure your addon's manifest is suitable, [more info here](/docs/reference/addon-manifest). Keep close attention to the name, description and tags of your addon. Make sure to set `"enabledByDefault"` to `false` or remove it. | ||
Make sure your addon doesn't break other addons. | ||
Make sure your code is understandable; having unnecessary comments is better than no comments. | ||
|
||
## Step 12: Send a pull request! | ||
Fork the repo if you haven't already, commit your new addon and send a PR! | ||
Keep in mind we might request you to make some changes, however, we will probably accept your addon as long as it's minimally suitable. | ||
--- | ||
title: Creating an Addon | ||
weight: 3 | ||
aliases: | ||
- /docs/developing/getting-started/creating-an-addon | ||
--- | ||
|
||
Required software: text editor, Chrome. | ||
If possible, disable the Scratch Addons extension you downloaded from the store before proceeding to avoid issues. | ||
|
||
## Step 1: Read about [addon basics](/docs/develop/getting-started/addon-basics/) | ||
|
||
Make sure to read that article to be familiar with the terminology. | ||
|
||
## Step 2: Fork/clone the repo | ||
|
||
Or download as ZIP, if you want. In other words, just download the source code locally. | ||
|
||
## Step 3: Load the extension into Chrome | ||
|
||
_Note: Chrome is recommended for working on addons. Nevertheless, addons are expected to work on Firefox and Edge._ | ||
Now that you have the extension in your filesystem, go to `chrome://extensions` and toggle "developer mode". | ||
Click "load unpacked", then select the folder where Scratch Addons is located. If you're having issues with this, make sure to be selecting the folder where `manifest.json` is located. | ||
That's it, you loaded the extension! It should look similar to this: | ||
![image](https://user-images.githubusercontent.com/17484114/91502527-accfd580-e89e-11ea-9e16-7daa2b808379.png) | ||
Note: you can safely ignore it says "errors". That's just a warning for an unrecognized manifest key that's required by Firefox. | ||
|
||
## Step 4: What's your addon about? | ||
|
||
Now comes the fun part! | ||
What will your addon do? Think of a self descriptive addon ID (no spaces or special characters, except hyphens). | ||
Got it? | ||
|
||
## Step 5: Create the folder for the addon | ||
|
||
Using a file explorer, go to the folder where Scratch Addons resides in your filesystem. Locate the `addons` folder. | ||
Then, create a new folder with your epic addon ID as its name. | ||
|
||
## Step 6: Add an addon manifest | ||
|
||
The addon manifest tells Scratch Addons how your addon works. Make sure to get this right to avoid headaches. | ||
Inside the folder you just created, create an `addon.json` file. | ||
This is a base you can use to start coding, make sure to change it in the future: | ||
|
||
```json | ||
{ | ||
"name": "Epic placeholder text in place of addon name", | ||
"description": "Hello World! It would be really smart to replace this placeholder text with a description.", | ||
"tags": ["community"], | ||
"enabledByDefault": false | ||
} | ||
``` | ||
|
||
For more information on what you can declare in the manifest, check [this article](/docs/reference/addon-manifest/). | ||
|
||
## Step 7: Tell Scratch Addons what your addon's ID is | ||
|
||
Scratch Addons can't find new folders by itself, so you have to add the name to a special file. | ||
Go to `scratchAddonsFolder/addons/addons.json` and add the ID of your addon to the array. | ||
|
||
## Step 8: Hello world | ||
|
||
Your addon does nothing at the moment, so it's a good time to check if everything we made previously worked. | ||
Go to `chrome://extensions` and reload Scratch Addons by clicking the refresh symbol on its card. | ||
Now, right-click the Scratch Addons icon, and click "options". | ||
You should see your addon on the list! Once you find it, enable it, and set any settings that you may have. | ||
|
||
## Step 9: The fun part, code! | ||
|
||
_Before proceeding, make sure you read the wiki article linked in step 1._ | ||
|
||
Here comes the fun part: create your own JS or CSS files! | ||
Protip: after making any change to your addon, make sure to refresh the Scratch Addons extension like you did in step 8. | ||
|
||
Depending on what you want your addon to do, you should now check these wiki pages: | ||
|
||
- [Userscripts](/docs/develop/addon-types/userscripts) | ||
- [Userstyles](/docs/develop/addon-types/userstyles) | ||
|
||
## Step 10: Make your addon customizable | ||
|
||
If you want, you can make your addon customizable! | ||
Users of your addon will be able to toggle settings, enter numbers, and more! | ||
To get started, see [how to declare settings in the addon manifest](/docs/reference/addon-manifest/#settings-object). | ||
Then, head to the [addon.settings documentation](/docs/reference/addon-api/addon.settings) to learn how to access user choices from userscripts. | ||
|
||
## Step 11: Before publishing your addon | ||
|
||
Now that your addon works, let's make sure we can add it to the addon library. | ||
Make sure your addon's manifest is suitable, [more info here](/docs/reference/addon-manifest). Keep close attention to the name, description and tags of your addon. Make sure to set `"enabledByDefault"` to `false` or remove it. | ||
Make sure your addon doesn't break other addons. | ||
Make sure your code is understandable; having unnecessary comments is better than no comments. | ||
|
||
## Step 12: Send a pull request! | ||
|
||
Fork the repo if you haven't already, create a new branch, commit your new addon, and send a PR! | ||
Keep in mind we might request that you make some changes. However, we will probably accept your addon as long as it's minimally suitable. | ||
To keep the process smooth and improve the chances of your PR being merged, you should review the [quality guidelines](/docs/develop/getting-started/quality-guidelines) that we abide by. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,38 @@ | ||||||
--- | ||||||
title: Quality Guidelines | ||||||
aliases: | ||||||
- /docs/developing/quality-guidelines | ||||||
--- | ||||||
Keep these guidelines in mind when creating or reviewing a pull request (PR). If you're not sure about a certain guideline when creating a PR, you can submit the PR as a draft and mention that you need help with a specific aspect of your PR. Many developers and community members are willing and able to help your PR improve! | ||||||
|
||||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd rather link https://scratchaddons.com/docs/getting-started/installing/ instead of that page. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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.** | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||
|
||||||
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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specify American English? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 work without any other addons enabled. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
to avoid confusion with the line above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding "also" seems okay to me |
||||||
|
||||||
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto from above There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 | ||||||
[] 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
|
||||||
[] I personally tested with no other addons enabled and did not find issues | ||||||
[] The addon is well-integrated with other addons | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
``` |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Sorry, something went wrong.