Ec36 avoid autoplay #40
Conversation
chihebtalan
force-pushed
the
ec36-avoid-autoplay
branch
from
9e1fab7 to
4f1705e
Compare
Co-authored-by: Diane Cordier <diane.cordier44@gmail.com>
chihebtalan
force-pushed
the
ec36-avoid-autoplay
branch
from
4f1705e to
5a3b699
Compare
Co-authored-by: Diane Cordier <diane.cordier44@gmail.com>
Co-authored-by: Diane Cordier <diane.cordier44@gmail.com>
| @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Add support for SonarQube up to 10.5 | |||
| - Add rule EC36 | |||
There was a problem hiding this comment.
Can you add an item in the changelog instead of editing this one?
|
|
||
| ⚠️ This rule _warns_ in the ✅ `recommended` config. | ||
|
|
||
| <!-- end auto-generated rule header --> |
There was a problem hiding this comment.
This generated header has been added twice.. can you remove one of it please?
(or all of them and re-run npm update script)
| EnforcePreloadNone: | ||
| "Set preload='none' for <video> and <audio> elements. Reference to Rule RGESN 4.1 : https://www.arcep.fr/mes-demarches-et-services/entreprises/fiches-pratiques/referentiel-general-ecoconception-services-numeriques.html", | ||
| NoAutoplay_EnforcePreloadNone: | ||
| "Avoid using autoplay attribute and set preload='none' for <video> and <audio> elements. Reference to Rule RGESN 4.1 : https://www.arcep.fr/mes-demarches-et-services/entreprises/fiches-pratiques/referentiel-general-ecoconception-services-numeriques.html ", |
There was a problem hiding this comment.
I like the idea of having multiple messages based on the context, awesome 👍
But I prefer to do not have the reference here, as this string will be the rule title used in SonarQube and it should be a way longer than accepted in the UI. Can you remove it from all messages?
| messageId: "EnforcePreloadNone", | ||
| type: "JSXAttribute", | ||
| }; | ||
| const expectedError3 = { |
There was a problem hiding this comment.
Can you rename these error variables to make them more explicit?
| @@ -43,7 +43,8 @@ public static List<Class<? extends JavaScriptCheck>> getAllChecks() { | |||
| NoMultipleStyleChanges.class, | |||
| PreferCollectionsWithPagination.class, | |||
| PreferShorthandCSSNotations.class, | |||
| ProvidePrintCSS.class | |||
| ProvidePrintCSS.class, | |||
| AvoidAutoPlay.class | |||
There was a problem hiding this comment.
Can you put it in an alphabetical order? This will prevent (I hope) some conflicts with other PRs
| @@ -1,6 +1,7 @@ | |||
| { | |||
| "name": "ecoCode", | |||
| "ruleKeys": [ | |||
| "EC13" | |||
| "EC13", | |||
| "EC36" | |||
There was a problem hiding this comment.
as the rule is both for JavaScript and TypeScript, you don't need to put it here (TypeScript-only rules)
There was a problem hiding this comment.
Even if the build passes thanks to the presence of this file, I propose to remove it from the PR in order to merge your work. But yes we will need to wait for the next release of ecocode-rules-specifications
utarwyn
added
🗃️ rule
No description provided.