feat(lint): add rule to limit excessive primary actions - #191
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds an ESLint rule limiting ChangesPrimary Action Linting
Feed Title Capitalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HTMLTemplate
participant lintTemplate
participant noExcessivePrimaryActions
participant ESLintContext
HTMLTemplate->>lintTemplate: parse HTML template
lintTemplate->>noExcessivePrimaryActions: process Document and button attributes
noExcessivePrimaryActions->>ESLintContext: report emphasis buttons beyond two
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
projects/core/eslint.config.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. projects/lint/src/eslint/internals/index.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. projects/lint/src/eslint/rules/no-excessive-primary-actions.test.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'.
Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds a new Elements ESLint rule to enforce a UX guideline: limit the number of “primary” (interaction="emphasis") <nve-button> actions to at most two within a page/template. It wires the rule into the recommended HTML config, updates downstream project ESLint overrides where needed, and documents the new rule.
Changes:
- Added new rule
@nvidia-elements/lint/no-excessive-primary-actionsand enabled it aserrorin the recommended HTML config. - Added unit tests for the rule and an integration-style assertion via
lintTemplate(...). - Updated lint documentation (docs site + package README) and disabled the rule for a specific Core visual test file.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/site/src/docs/lint/index.md | Documents the new rule in the docs site lint rule list. |
| projects/lint/src/eslint/rules/no-excessive-primary-actions.ts | Implements the new rule (counts interaction="emphasis" on <nve-button>). |
| projects/lint/src/eslint/rules/no-excessive-primary-actions.test.ts | Adds rule-level tests for valid/invalid emphasis button counts. |
| projects/lint/src/eslint/internals/index.test.ts | Adds coverage that lintTemplate(..., { strict: true }) surfaces the new message id. |
| projects/lint/src/eslint/configs/html.ts | Registers and enables the new rule in the recommended HTML config/plugin. |
| projects/lint/README.md | Documents the new rule in the lint package README rule table. |
| projects/core/eslint.config.js | Disables the new rule for src/button/button.test.visual.ts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('should count separate tagged templates independently', () => { | ||
| const javascriptTester = new RuleTester({ | ||
| languageOptions: { | ||
| ecmaVersion: 'latest', | ||
| sourceType: 'module' | ||
| } | ||
| }); | ||
|
|
||
| javascriptTester.run('separate tagged templates', rule, { | ||
| valid: [ | ||
| `const first = html\` | ||
| <nve-button interaction="emphasis">One</nve-button> | ||
| <nve-button interaction="emphasis">Two</nve-button> | ||
| \`; | ||
| const second = html\` | ||
| <nve-button interaction="emphasis">Three</nve-button> | ||
| <nve-button interaction="emphasis">Four</nve-button> | ||
| \`;` | ||
| ], | ||
| invalid: [] | ||
| }); | ||
| }); |
| '@nvidia-elements/lint/no-deprecated-global-attribute-value': ['error'], | ||
| '@nvidia-elements/lint/no-deprecated-global-attributes': ['error'], | ||
| '@nvidia-elements/lint/no-deprecated-slots': ['error'], | ||
| '@nvidia-elements/lint/no-excessive-primary-actions': ['error'], |
There was a problem hiding this comment.
went with slightly more generic name so the rule has some flexibility to what it checks
1ec8017 to
b0254f0
Compare
b0254f0 to
2794c0e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
projects/lint/src/eslint/rules/no-excessive-primary-actions.test.ts:89
- This test configures a RuleTester for JavaScript using the default JS parser, but the rule is intended to run under
@html-eslint/parser(the repo’s HTML config usesparser: htmlParsereven for*.js/ts/tsx). As written, this test may not reflect how the rule behaves in real usage, and could pass while the real config behaves differently.
const javascriptTester = new RuleTester({
languageOptions: {
ecmaVersion: 'latest',
sourceType: 'module'
}
});
Signed-off-by: Cory Rylan <crylan@nvidia.com>
- Introduced a new ESLint rule `@nvidia-elements/lint/no-excessive-primary-actions` to restrict the use of emphasis buttons to a maximum of two per page. - Updated ESLint configuration to include the new rule. - Added tests for the new rule to ensure correct functionality. - Updated documentation to reflect the new rule and its purpose. Signed-off-by: Cory Rylan <crylan@nvidia.com>
2794c0e to
afa8d69
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
projects/lint/src/eslint/rules/no-excessive-primary-actions.test.ts:89
- This test’s
javascriptTesteruses ESLint’s default parser (Espree), but the rule relies on @html-eslint’s AST/visitors and is configured in this repo to run under@html-eslint/parsereven for .js/.ts/*.tsx. With the default parser, the HTML visitors may never run, making the test ineffective or failing. Configure this tester to usehtmlParser(consistent with other rule tests).
const javascriptTester = new RuleTester({
languageOptions: {
ecmaVersion: 'latest',
sourceType: 'module'
}
});
|
🎉 This issue has been resolved in version 2.2.0 🎉 |
@nvidia-elements/lint/no-excessive-primary-actionsto restrict the use of emphasis buttons to a maximum of two per page.Summary by CodeRabbit
<nve-button interaction="emphasis">elements.