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.

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
180 changes: 98 additions & 82 deletions content/docs/develop/getting-started/creating-an-addon.md
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.
37 changes: 37 additions & 0 deletions content/docs/reference/quality-guidelines.md
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

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

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.

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
@@ -0,0 +1,37 @@
---
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.
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.**
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

What I meant on this point was what Maximouse said.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Agreed on making it a recommendation.

Copy link
Member

@DNin01 DNin01 Jan 20, 2023

Choose a reason for hiding this comment

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

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.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

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.

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 work without any other addons enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

to avoid confusion with the line above

Copy link
Member

Choose a reason for hiding this comment

The 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
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

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
[] I personally tested with no other addons enabled and did not find issues
[] 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
```