|
| 1 | +# GitHub Copilot Review Instructions for UI5 Web Components |
| 2 | + |
| 3 | +When reviewing pull requests to UI5 Web Components project, follow these guidelines: |
| 4 | + |
| 5 | +## Commit Message and PR Title Validation |
| 6 | + |
| 7 | +Check that commit messages and PR titles follow the [Conventional Commits](https://conventionalcommits.org) specification as outlined in [Conventions and guidelines](../docs/5-contributing/02-conventions-and-guidelines.md): |
| 8 | + |
| 9 | +### Required Format |
| 10 | +``` |
| 11 | +<type>(<scope>): <description> |
| 12 | +
|
| 13 | +[optional body] |
| 14 | +
|
| 15 | +[optional footer] |
| 16 | +``` |
| 17 | + |
| 18 | +### Valid Types |
| 19 | +- `fix` - bug fix (triggers release) |
| 20 | +- `feat` - new feature (triggers release) |
| 21 | +- `docs` - documentation changes |
| 22 | +- `style` - formatting changes (no code meaning change) |
| 23 | +- `refactor` - code change that neither fixes bug nor adds feature |
| 24 | +- `perf` - performance improvement |
| 25 | +- `test` - adding missing tests |
| 26 | +- `chore` - build process or auxiliary tool changes |
| 27 | +- `revert` - revert to a commit |
| 28 | +- `WIP` - work in progress |
| 29 | + |
| 30 | +### Scope (optional) |
| 31 | +- Should point to specific component (e.g., `ui5-button`, `ui5-card`, `ui5-table`) |
| 32 | + |
| 33 | +### Description Requirements |
| 34 | +- Use imperative present tense lowercase ("add" not "Added", "added" or "Adding", "adding) |
| 35 | +- Maximum 100 characters |
| 36 | +- No period at the end |
| 37 | + |
| 38 | +### Example Valid Commit |
| 39 | +``` |
| 40 | +fix(ui5-button): correct focus with 'tab' key |
| 41 | +
|
| 42 | +The button should receive a correct focus outline |
| 43 | +when the 'tab' key is pressed. |
| 44 | +
|
| 45 | +Fixes #42 |
| 46 | +``` |
| 47 | + |
| 48 | +## Description Requirements |
| 49 | + |
| 50 | +Ensure the PR includes: |
| 51 | +- Clear description of what changed and why |
| 52 | +- For breaking changes, include `BREAKING CHANGE:` in footer with description |
| 53 | +- Context about the problem being solved |
| 54 | +- (optional) Reference to related GitHub issues using `Fixes #<issueNumber>` format |
| 55 | + |
| 56 | +## Code Quality Analysis |
| 57 | + |
| 58 | +### Component Development (refer to [DoD.md](../docs/5-contributing/03-DoD.md)) |
| 59 | + |
| 60 | +**Properties:** |
| 61 | +- ✅ For public properties, JSDoc is added (since tag is present) |
| 62 | +- ✅ Check that public properties are not changed without user interaction |
| 63 | +- ✅ If public properties are changed, a public event should be fired to notify the consumers |
| 64 | +- ✅ Verify `noAttribute: true` is set for private properties not used in CSS |
| 65 | +- ✅ Ensure CSS handles default values for enum properties |
| 66 | + |
| 67 | +**Events:** |
| 68 | +- ✅ Verify proper event handling and naming conventions |
| 69 | +- ✅ Check event documentation and examples |
| 70 | + |
| 71 | +**CSS:** |
| 72 | +- ✅ Confirm styling properties are placed on `:host` when possible for app overriding |
| 73 | +- ✅ Check responsive design considerations |
| 74 | + |
| 75 | +**Scoping/Multi-framework Safety:** |
| 76 | +- ❌ **Flag hard-coded tag names** in CSS/TS files - use attribute notation instead: |
| 77 | + ```css |
| 78 | + /* BAD */ |
| 79 | + ui5-button.accept-btn { color: green; } |
| 80 | + |
| 81 | + /* GOOD */ |
| 82 | + [ui5-button].accept-btn { color: green; } |
| 83 | + ``` |
| 84 | +- ❌ **Flag `instanceof` checks** for UI5 Elements - use duck-typing instead |
| 85 | +- ✅ Verify all required icons are explicitly imported |
| 86 | +- ✅ Check for attribute notation in `shadowRoot.querySelector` calls |
| 87 | + |
| 88 | +### General Code Issues |
| 89 | +- ❌ **Memory leaks** - check for proper cleanup of event listeners |
| 90 | +- ❌ **Accessibility issues** - verify ARIA attributes and keyboard navigation |
| 91 | +- ❌ **Performance problems** - unnecessary re-renders or heavy computations |
| 92 | +- ❌ **Type safety** - proper TypeScript usage and type definitions |
| 93 | +- ❌ **Security vulnerabilities** - XSS risks, unsafe DOM manipulation |
| 94 | + |
| 95 | +## Test Coverage Requirements |
| 96 | + |
| 97 | +### Required Tests |
| 98 | +- ✅ **Cypress tests** for new components or changed functionality |
| 99 | + |
| 100 | +### Test Frameworks |
| 101 | +- **Cypress** for end-to-end testing with component interaction |
| 102 | +- Custom WDIO configuration with UI5-specific hooks |
| 103 | + |
| 104 | +### Test Quality Checks |
| 105 | +- ❌ **Flag framework-testing code** - tests should verify component behavior, not framework features |
| 106 | +- ✅ Ensure tests cover the actual changes made |
| 107 | +- ✅ Verify mobile testing when applicable |
| 108 | + |
| 109 | +## Review Checklist |
| 110 | + |
| 111 | +Before approving, verify: |
| 112 | + |
| 113 | +1. **Commit message follows conventional commits format** |
| 114 | +2. **PR description explains the change and references issues** |
| 115 | +3. **Code follows project conventions and guidelines** |
| 116 | +4. **No scoping-unsafe code patterns** |
| 117 | +5. **Proper test coverage included** |
| 118 | +6. **No obvious bugs or security issues** |
| 119 | +7. **TypeScript definitions updated if needed** |
| 120 | +8. **Documentation updated for public API changes** |
| 121 | + |
| 122 | +## Common Patterns to Flag |
| 123 | + |
| 124 | +### ❌ Unsafe Patterns |
| 125 | +```typescript |
| 126 | +// Hard-coded tag names |
| 127 | +this.shadowRoot.querySelector("ui5-popover") |
| 128 | + |
| 129 | +// instanceof checks |
| 130 | +if (element instanceof Button) { } |
| 131 | + |
| 132 | +// Public property modification without user interaction |
| 133 | +this.myPublicProperty = "newValue" |
| 134 | +``` |
| 135 | + |
| 136 | +### ✅ Safe Patterns |
| 137 | +```typescript |
| 138 | +// Attribute notation |
| 139 | +this.shadowRoot.querySelector("[ui5-popover]") |
| 140 | + |
| 141 | +// Duck-typing |
| 142 | +if (element.tagName === "UI5-BUTTON") { } |
| 143 | + |
| 144 | +// Property modification in event handlers only |
| 145 | +onUserInteraction() { |
| 146 | + this.myPublicProperty = "newValue" |
| 147 | + this.fireEvent("change") |
| 148 | +} |
| 149 | +``` |
| 150 | + |
| 151 | +## Additional Resources |
| 152 | + |
| 153 | +- [Development Workflow](../docs/5-contributing/01-development-workflow.md) |
| 154 | +- [Conventions and Guidelines](../docs/5-contributing/02-conventions-and-guidelines.md) |
| 155 | +- [Definition of Done](../docs/5-contributing/03-DoD.md) |
| 156 | +- [Testing Documentation](../commitlint.config.cjsdocs/4-development/10-testing.md) |
0 commit comments