feat(ui5): Add accessibility best practices skill#85
Conversation
|
|
- SKILL.md: drop non-standard `allowed-tools` line; rewrite topic-file paths to plain `references/X.md` to match sibling skills; expand description with WCAG / screen-reader / focus-handling keywords; rename topic 4 to "Focus & keyboard" so it matches keyboard.md. - tests/fixtures: add gap-keyboard.view.xml covering all three topic-4 violations (missing initialFocus, missing sap-ui-fastnavgroup, tabindex > 0) — this was the one topic without a gap fixture. - tests/README.md: register the new fixture and clarify that fixtures are run manually (no auto harness).
GerganaKremenska
force-pushed
the
ui5-accessibility-skill
branch
from
94c1833 to
e18447e
Compare
There was a problem hiding this comment.
LGTM
An UA review and Acc expert review is still nice to have!
From my perspective structure & content wise the skill is good to go!
I got something that might improve the accuracy and might worth give it a try:
- create an
eval.mdfile, so that test cases are evaluated correctly and the model won't rely simply on test case's name to identify what to expect.
Here are some resources I found: https://www.mindstudio.ai/blog/self-improving-marketing-skill-claude-code-eval-json
Defines a versioned eval suite for the ui5-best-practices-accessibility skill. Each of the 14 fixtures gets a test case with binary assertions (report_finding, report_empty, finding_count_max, must_not_mention) that a runner can check against the skill's Step-3 report. Follows the eval.json pattern described at https://www.mindstudio.ai/blog/self-improving-marketing-skill-claude-code-eval-json
added eval.json |
All 33 assertions across 14 test cases pass at first attempt. The eval.json harness is well-designed — must_not_mention assertions on the ok-fixtures directly guard the three highest false-positive risk scenarios (SimpleForm, Dialog customHeader, ObjectNumber). Well done! 👍 |
LilyanaOviPe
left a comment
LilyanaOviPe
left a comment
There was a problem hiding this comment.
Just a few minor comments. Otherwise looks good.
| # Skill Test Fixtures | ||
|
|
||
| These fixtures are **not** auto-run — there is no test harness, CI job, or assertion | ||
| framework. They are reference cases for manually validating the skill's behaviour: |
There was a problem hiding this comment.
| framework. They are reference cases for manually validating the skill's behaviour: | |
| framework. They are reference cases for manually validating the skill's behavior. |
There was a problem hiding this comment.
Applied in 92be973 — "behaviour" → "behavior".
| invoke `/ui5-best-practices-accessibility` against each file and verify the output | ||
| matches the expectations below. |
There was a problem hiding this comment.
| invoke `/ui5-best-practices-accessibility` against each file and verify the output | |
| matches the expectations below. | |
| Invoke `/ui5-best-practices-accessibility` against each file and verify the output | |
| matches the expectations below. |
There was a problem hiding this comment.
Applied in 92be973 — capitalized as the start of a new sentence.
| ## When not to use | ||
|
|
||
| - Do not provide information exclusively for AT users — screen reader users should not receive content that sighted users cannot access | ||
| - Do not use it to hide long texts — if the information matters, show it visibly |
There was a problem hiding this comment.
| - Do not use it to hide long texts — if the information matters, show it visibly | |
| - Do not use it to hide long texts — if the information matters, show it |
There was a problem hiding this comment.
Applied in 92be973 — dropped "visibly".
- Use US spelling: behaviour → behavior in tests/README.md - Capitalize "Invoke" as start of a new sentence - Trim redundant "visibly" tail in invisible-message.md
|
|
||
| **Wrong:** | ||
| ```xml | ||
| <Title text="Order Details"/> |
There was a problem hiding this comment.
I would recommend to use bindings e.g. {i18n>titleText} instead of hard coded texts displayed on the UI.
There was a problem hiding this comment.
Applied in 6f27c79 — converted all hard-coded UI-facing strings across the 8 accessibility reference files to {i18n>...} bindings (XML) and oResourceBundle.getText(...) reads (JS). Each reference file now also notes the i18n convention up front.
| - **Personalization** - `sap.m.p13n.Engine` integration | ||
| - **Cell templates & alignment** - Type-based alignment and model type namespace rules | ||
|
|
||
| #### ui5-best-practices-accessibility |
There was a problem hiding this comment.
please move this section above the integration cards section to ensure alphabetical sorting
There was a problem hiding this comment.
Applied in 6f27c79 — moved the ui5-best-practices-accessibility section above integration-cards to keep the specific-topic sections alphabetical.
- Move ui5-best-practices-accessibility section above integration-cards in
plugins/ui5/README.md to keep the specific-topic sections alphabetical
- Convert all hard-coded UI-facing strings in the 8 accessibility reference
files to `{i18n>...}` bindings, matching UI5 i18n best practices. XML
snippets bind text/title/tooltip/alt/label properties via the resource
model; JS snippets read from oResourceBundle.getText(). Each reference
now includes a one-line note that all snippets bind via i18n.
|
LGTM |
5 participants
Adds a new skill for UI5 accessibility best practices covering keyboard navigation, labelling, landmarks, heading levels, reading order, target size, shortcuts, and invisible messages. Includes reference documentation and test fixtures for each topic.