Skip to content

feat(ui5): Add SAP Fiori guidelines skill - #129

Open
hinzzx wants to merge 1 commit into
mainfrom
ui5/fiori-guidelines
Open

hinzzx wants to merge 1 commit into
mainfrom
ui5/fiori-guidelines

Conversation

@hinzzx

@hinzzx hinzzx commented Aug 31, 2026

Copy link
Copy Markdown

What

Adds a new ui5-fiori-guidelines skill to the ui5 plugin.

It helps pick the right SAP Fiori / UI5 control for a given use case, and flags the wrong one before it ships.

Why

The most common UI5 mistake isn't syntax, it's reaching for the wrong control.

A sap.m.Bar used as an app header, a responsive table handed thousands of rows, a hand-built Dialog where a MessageBox belongs. These compile, look fine in review, then bite later as UX and performance problems.

The existing skills cover how to write UI5 well. None of them covered which control to use in the first place. This fills that gap.

What's in it

A SKILL.md router with a Wrong to Right table, a use-case to component map, and a set of short one-line rules.

For each control it gives both names, the SAPUI5 class and the ui5-* web component tag, and says so explicitly when a control only exists in one of them.

Eleven reference files under references/, read on demand, one per topic (actions, inputs, containers, navigation, lists and tables, display, messages, AI, upload, theming tokens, and a review checklist).

A REFRESH.md that documents how to re-sync the skill against SAP sources. It reports drift and proposes edits, a human reviews and applies. No unattended writes.

Notes for reviewers

The skill loads automatically through the plugin's skills/ directory scan, so no plugin.json change was needed.

Also we've reworked and restructured the below-mentioned skill a little bit, as it appears we've had similar ideas.

Credits:
https://github.com/SAP/ai-skills-library/tree/main/skills/sap-fiori-guidelines

@flovogt

flovogt commented Sep 1, 2026

Copy link
Copy Markdown
Member

I would like to also mention "OpenUI5" in the skill. So, controls only available in SAPUI5 should be marked with "SAPUI5". Controls available in both with "OpenUI5 / SAPUI5".

@s-todorova s-todorova left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Some Rodpoi controls are missing entries. Proposals:

## Carousel

- **SAPUI5:** `sap.m.Carousel`  **Web Component:** `ui5-carousel` (`@ui5/webcomponents`)
- **Use when:** Users browse a set of items by paging/swiping back and forth — image galleries, sequential display, or several items shown side by side.
- **Do NOT use when:** All items need to be visible at the same time → use a grid/list layout; the items are very similar and paging adds no value; displaying a single static image → use `sap.m.Image`.
- **Rules:**
  - Place navigation arrows either in the content area (appear on hover) or beside the page indicator — pick one; do not show both.
  - Navigation buttons are never shown on touch devices; users swipe instead. Do not rely on arrows as the only way to page.
  - Let the page indicator style follow the default: dots for fewer than 9 pages, numeric ("2 of 4") for 9 or more. Only override with reason.
  - Keep width/height at 100% (default) so the content area adapts to available space.
  - Set cyclic/loop only when wrap-around from last to first page is expected behavior.
- **Gotchas:** On touch devices the arrows disappear entirely — swipe is the only gesture, so keyboard/arrow-only interaction models break there. Arrow direction and looping automatically flip in right-to-left languages; do not hardcode direction. The page indicator wraps to multiple lines if too long to fit.
## Tag

- **SAPUI5:** `sap.m.ObjectStatus` (closest SAPUI5 equivalent for semantic status text)  **Web Component:** `ui5-tag` (`@ui5/webcomponents`)
- **Use when:** Showing complementary information about an object — a status, category, quantity, KPI, or condition — in a compact form, with optional icon and/or text.
- **Do NOT use when:** Representing entered values in an input → use tokens in `ui5-multi-input` / `ui5-multi-combobox`; showing a count on a button → use a button badge; the label is a single letter or number → a tag has a minimum width and is not meant for that.
- **Rules:**
  - Keep text short — avoid long strings; tags are complementary, not primary content.
  - Use `Small` (default) in tables and lists for general status; `Large` in object headers.
  - Use semantic value states (`design`) to color-code state; use custom colors only when no semantic meaning applies.
  - Icon-only design: use exactly one icon.
  - Do not overuse tags on a page — too many dilute their attention-directing purpose.
- **Gotchas:** The `Active` state is only for progressive disclosure (e.g. opening a popover with more detail) — do not make a tag interactive for anything else. Text wraps by word or truncates with an ellipsis depending on configuration; a single character will not render at expected width due to the minimum-width rule.
## Expandable Text

- **SAPUI5:** `sap.m.ExpandableText`  **Web Component:** `ui5-expandable-text` (`@ui5/webcomponents`)
- **Use when:** Long text or descriptions must fit a responsive table, list, or form without dominating the layout — show a truncated preview with a "Show More" link that expands inline or in a popover.
- **Do NOT use when:** The text sits in a grid/tree/analytical table or Gantt chart → not supported there; a short, meaningful alternative exists → use that instead; the content is critical → use a short description that fits the available space rather than hiding it behind a link.
- **Rules:**
  - Tune the truncation length to the use case (default: 100 characters).
  - Choose the display mode deliberately: inline expansion, or a popover for longer content.
  - If using the popover mode, size the popover to the text length so it neither overflows nor leaves large empty space.
  - In tables, set minimal padding so expanded rows do not gain extra whitespace.
- **Gotchas:** On size S (phones) the popover mode covers the full screen instead of floating. The "Show More" link becomes "Show Less" when expanded; both flip to the reading-direction end (right for LTR, left for RTL) automatically — do not hardcode link position.

Should also be mentioned that the following are not covered as standalone entries: sap.m.Text/ui5-text, sap.m.Title/ui5-title, sap.m.Label/ui5-label, but perhaps it is not needed. Their usage is self-evident (body text, headings, field labels) and the only non-obvious rules — pair every input with a Label, use Title not Label for table/column headers — are enforced inline where relevant.


- **SAPUI5:** `sap.m.BusyIndicator` **Web Component:** `ui5-busy-indicator` (`@ui5/webcomponents`)
- **Use when:** An operation is in progress and duration is unknown; only part of the UI is affected and the user can still interact with the rest.
- **Do NOT use when:** The operation takes less than 1 second (avoid flicker); you need to block the entire screen → use a `sap.m.Dialog` with `busyIndicatorDelay`; multiple concurrent operations → show one indicator, not several.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here it could be mentioned that for SAPUI5, a premade sap.m.BusyDialog exists.

4. **Visual cards/tiles, not columns?** → Grid List.
5. **Otherwise** → Responsive Table / `ui5-table`.

Smart/MDC tables: `sap.ui.comp.smarttable.SmartTable` (V2) / `sap.ui.mdc.Table` (V4) are annotation-driven wrappers. Do not hand-build these.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ODataV2 / ODataV4

| General-purpose, mobile + desktop, simple, up to ~200 rows | Responsive Table | `sap.m.Table` | `ui5-table` (popin mode) |
| Large data, many columns, desktop-first, cell comparison | Grid Table | `sap.ui.table.Table` | none |
| Aggregation: sums/subtotals per group (OData analytical binding) | Analytical Table | `sap.ui.table.AnalyticalTable` | none |
| True parent-child hierarchy (BOM, org chart, cost centers) | Tree Table | `sap.ui.table.TreeTable` | none |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is BOM clear, or should we use "Bill of Meterial"?

Smart/MDC tables: `sap.ui.comp.smarttable.SmartTable` (V2) / `sap.ui.mdc.Table` (V4) are annotation-driven wrappers. Do not hand-build these.

Common misuses:
- **Responsive Table for 5,000 rows** → DOM bloat, single-column pop-in mush. Use Grid Table.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would use "large and complex data" instead of "5000 rows". The number of rows that the table can handle was already mentioned before as ~200.


Common misuses:
- **Responsive Table for 5,000 rows** → DOM bloat, single-column pop-in mush. Use Grid Table.
- **Analytical Table with no aggregation** as a "nicer grid" → adds cost, breaks mobile. Use Grid Table.

@stoyanovski stoyanovski Sep 16, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

as a "nicer grid" is not needed. If there are no aggregations, the table will look exactly the same as a Grid Table


## Responsive Table

- **SAPUI5:** `sap.m.Table` **Web Component:** `ui5-table` (`@ui5/webcomponents`)

@stoyanovski stoyanovski Sep 16, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shouldn't SAPUI5 be used for the SmartTable only? The other tables are available in OpenUI5.

- **Rules:**
- Always pair with a toolbar (`sap.m.OverflowToolbar`) — the grid table does not have a built-in title area.
- For column personalization with many columns, use `sap.m.p13n.Popup` (P13n Dialog), not `sap.m.ViewSettingsDialog`.
- Use the multi-selection plug-in (`sap.ui.table.plugins.MultiSelectionPlugin`) over multi-toggle mode for reliable select-all behavior with server-side data.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

multi-toggle mode -> MultiToggle selectionMode

## Tables and Lists

- [ ] Responsive table (`sap.m.Table`) used only for ≤~200 rows, no aggregation needed
- [ ] Large datasets or totals use `sap.ui.table.AnalyticalTable` — not responsive table

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The second rule can be splitted:

  • For large data sets use sap.ui.table.Table
  • For subtotals and totals use sap.ui.table.AnalyticalTable

| "inline error / warning banner" | `sap.m.Dialog` / `MessageBox` | `sap.m.MessageStrip` · `ui5-message-strip` | Dialogs interrupt; strips are non-blocking, stay on the page. |
| "brief success confirmation" | `sap.m.MessageStrip` / `Dialog` | `sap.m.MessageToast` · (no WC — use `ui5-toast`) | Toast auto-dismisses; a strip persists and clutters. |
| "many validation errors at once" | repeated `MessageStrip` | `sap.m.MessagePopover` · (no direct WC) | One entry point, grouped by severity, navigable. |
| "big data table", "needs totals/subtotals" | `sap.m.Table` | `sap.ui.table.AnalyticalTable` · (no WC) | Responsive table has no aggregation/freeze and degrades past ~200 rows. |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Same here the rule can be splitted: big data table -> sap.ui.table.Table, needs totals/subtotals -> sap.ui.table.AnalyticalTable

4. **Visual cards/tiles, not columns?** → Grid List.
5. **Otherwise** → Responsive Table / `ui5-table`.

Smart/MDC tables: `sap.ui.comp.smarttable.SmartTable` (V2) / `sap.ui.mdc.Table` (V4) are annotation-driven wrappers. Do not hand-build these.

@stoyanovski stoyanovski Sep 21, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P13n should not be hand-build either. The sap.m.p13n library should be used instead

- Use `ui5-table-header-row` + `ui5-table-header-cell` + `ui5-table-row` + `ui5-table-cell` — do not put raw `<tr>`/`<td>` inside.
- Selection features must be imported separately: `TableSelectionMulti`, `TableSelectionSingle` (WC).
- `rememberSelections` (SAPUI5 property) should be `false` when a Filter Bar triggers search — reset selections on each search.
- Show no more columns than fit ~80% of use cases; use pop-in / column hide for the rest.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A general question: That's a wild mixture of infos about the APIs of the WebComponent Table and the ResposniveTable in SAPUI5. Will the API not be confused when we do not clearly separate both worlds. Concepts are similar but not necessarily the same and esp. the APIs are partially different.
This comment might affect other places as well.

@plamenivanov91 plamenivanov91 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hello Stoyan,

Here are some comments from team Pirin's perspective.

If we strip subsidiary controls (e.g. AnchorBar), all non standalone and all deprecated/unused ones, these are the Team Pirin controls with no guidance in this skill:

  • sap.f.DynamicPage
  • sap.f.ProductSwitch
  • sap.m.GenericTag
  • sap.m.DraftIndicator
  • sap.m.Image
  • sap.m.PullToRefresh
  • sap.m.App / sap.m.NavContainer (if shell-level guidance is in scope)

Also FlexibleColumnLayout is vaguely mentioned. It's a complex control with a lot of guidelines which seem to be missing.

Additionally I believe that every control should have its own file. This way, the LLM won't waste time/tokens to probe the bigger files like ui-actions.md. That being said, the latter might stay as further guidance (when needed).

I am talking about similar file structure:

Image

It would be ideal to be able to fetch the guidelines live while using the skill but this is not feasible due to 403 Forbidden status. And because of this, we will have to maintain this skill in addition to the guidelines themselves. May be we should make some automation in the future where the skill will get updated regularly or when guidelines are changed.

Your PR code base is not ISO 8859-1 (SAP supported standard) compliant.

I guess we don't have different guidelines for UI5 Web C and OpenUI5 but components and controls are often quite different (Jens mentioned something similar in above comment). Both in feature set and sometimes in rendering look and feel.

Here are some additional comments which Sonnet 4.6 found:

A few more things I noticed now that I've read everything:

1. Factual inconsistency — SideNavigation library
ui-navigation.md correctly says sap.tnt.SideNavigation, but component-selection.md:109 says sap.f.SideNavigation — that namespace doesn't exist (or is long deprecated). An LLM will generate the wrong import from that.

2. Skill scope is actually two skills in one
The description says "picks the correct control" but implementation-checklist.md is a full app-review tool — a different task triggered differently. These competing purposes can cause the skill to be invoked too broadly or not at all for the review case. Worth either splitting or making the checklist a separate explicit trigger.

3. No eval suite
The accessibility skill and qunit skill both have tests/eval.json. This skill has none — there's no way to measure whether the wrong→right table actually catches mistakes reliably, or whether the reference dispatch works. For a skill this broad it's the biggest quality risk.

4. Topic file grouping is inconsistent
ShellBar and OverflowToolbar are in ui-containers.md but they're navigation and actions controls respectively. This will become a maintenance problem over time — contributors won't agree on where to put new controls, and the per-control structure you discussed would naturally fix this.

The SideNavigation library error is the only one I'd flag as a bug to fix in this PR. The rest are architectural observations like the ones you're already posting.

Regards,
Plamen

@hinzzx

hinzzx commented Sep 24, 2026 •

Copy link
Copy Markdown
Author

Hi colleagues,

Thank you for thorough and detailed comments! I would like to mention that the Web Components part of this skill will be completely removed, so OpenUI5 and SAPUI5 ones will only be left here. So for future colleagues, that will review the change — you can skip the Web C part.

@plamenivanov91 - I also think that, a per control skill/instruction file would be the better approach! However it would've been extremely time consuming on our part to do it ourselves. Therefore, we decided to move forward with an MVP skill, where other teams will contribute as well and incrementally develop and grow the skill.

We also removed some components/controls consciously, as we were not sure if they classified as part of the scope of the task. This is why we needed your input guys as well.

I am currently addressing comments, and will make sure to address every concern. Again, thank you for your inputs!

- Add a close button (`hide-close-button="false"`) for temporary/informational messages; omit it for persistent system-state messages.
- Show an icon (default: on) — only hide it when a custom icon via the `icon` slot is used instead.
- Keep text concise; use inline links (`ui5-link`) only to point to related details, not as primary actions.
- **Gotchas:** `design="ColorSet1"` / `"ColorSet2"` do not render a default icon — you must supply one via the `icon` slot. The strip has no WC `valueState` prop; semantic color is set via `design`, not `value-state`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ColorSet1/ColorSet2 are values of the colorSet property (added in 1.143.0), not design. The underlying fact (no default icon for ColorSet variants) is correct, only the property name is wrong.

- **Use when:** Displaying a persistent, non-blocking message scoped to a page, section, or single component (e.g., a table) that the user needs to see while working.
- **Do NOT use when:** The message requires a decision or interruption → use `sap.m.MessageBox`; the message is a brief, transient success confirmation → use `sap.m.MessageToast` / `ui5-toast`; there are many validation errors → use `sap.m.MessagePopover`.
- **Rules:**
- Always set `design` explicitly: `Negative`, `Critical`, `Positive`, or `Information`.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The property is called "type" not "design".

@Vonahz

Vonahz commented Oct 12, 2026

Copy link
Copy Markdown

Some Rila missing controls that may be worth including:

  • sap.m.RatingIndicator (ui5-rating-indicator) for references/ui-display.md
  • sap.m.Token / sap.m.Tokenizer (ui5-token / ui5-tokenizer) for references/ui-display.md
  • sap.m.Panel (ui5-panel) for references/ui-containers.md
  • sap.m.Wizard (ui5-wizard) for references/ui-containers.md

Suggestions:

## RatingIndicator

- **SAPUI5:** `sap.m.RatingIndicator` 
- **Use when:** Collecting or displaying a star-based rating — product reviews, quality assessments, feedback forms. Also suitable for displaying averaged ratings (e.g. 3.5 stars).
- **Do NOT use when:** Showing completion progress of a known process → `ui5-progress-indicator`; showing a numeric score with precise increments → `ui5-step-input`; binary feedback → `ui5-checkbox` or `ui5-switch`.
- **Rules:**
  - Keep `maxValue` between 5 (default) and 7 — the recommended range from the Fiori design.
  - Use `displayOnly: true` for review-mode forms — removes the control from the tab chain entirely.
  - Use `editable: false` when the control should be focusable but not interactive (e.g. screen-reader friendly read-only).
  - `visualMode="Half"` (default) displays half-star averages visually — users cannot interactively select half values.
  - Listen to `change` (fires on final selection, value type `int`) for form submission; avoid relying on `liveChange` (fires continuously during drag, type `float`).
  - Do not use XS `iconSize` for editable indicators — it is too small for touch targets.
- **Gotchas:** Default state is **editable** — always set `editable: false` or `displayOnly: true` explicitly for display-only use. `displayOnly` and `editable: false` are distinct: `displayOnly` removes from the tab chain; `editable: false` keeps focus but disables interaction.
## Token / Tokenizer

- **SAPUI5:** `sap.m.Token` / `sap.m.Tokenizer`.
- **Use when:** Visualizing selected values as dismissible chips. `sap.m.Tokenizer` can be used standalone as a display/form control; ``sap.m.Token` can only be used inside `sap.m.Tokenizer` or directly in `sap.m.MultiInput`.
- **Do NOT use when:** You need standalone tag or badge components → use `sap.m.ObjectStatus`; you need a non-dismissible visual label → use `sap.m.Label`.
- **Rules:**
  - Always set both `key` (identifier) and `text` (display label) on `sap.m.Token` — key is required for deletion and restore logic.
  - Listen to Tokenizer's `tokenDelete` event and manually remove or destroy the token from the aggregation — the Tokenizer does **not** auto-remove tokens after this event fires.
  - Do not use `tokenChange` or `tokenUpdate` — both are deprecated, use `tokenDelete` instead.
  - For review-mode forms: set `displayOnly: true` on the Tokenizer — implies non-editable and hides delete icons.
- **Gotchas:** In `renderMode: Narrow` (default), overflow tokens are hidden behind an "N more" indicator that opens a `ResponsivePopover` on click — this is expected behavior, not a bug.
## Panel

- **SAPUI5:** `sap.m.Panel`
- **Use when:** Grouping related form fields or content in a collapsible section — structural grouping with an optional expand/collapse toggle.
- **Do NOT use when:** You need a data-presentation unit with header actions → use `sap.m.Card` / `ui5-card`; for a modal overlay → use Dialog; you are designing an Object Page — never use panels in the Object Page content area; do not nest panels inside panels and avoid stacking too many panels on one page.
- **Rules:**
  - `expandable: false` (default) renders the panel always expanded — set `expandable: true` to enable the expand/collapse toggle.
  - `headerToolbar` overwrites `headerText` — use one or the other, not both.
  - For non-form content change `accessibleRole` from the default `"Form"` to `"Region"` or `"Complementary"`.
  - `stickyHeader: true` keeps the header visible during scroll (forces solid background design).
  - The `expand` event fires **before** the panel expands or collapses; use the `triggeredByInteraction` parameter to distinguish user interaction from a programmatic `setExpanded()` call.
- **Gotchas:** A fixed `height` is preserved even when the panel is collapsed — a collapsed panel with a fixed height will render as an empty fixed-height header box. Use `height: auto` (default) to let the content area resize automatically.
## Wizard

- **SAPUI5:** `sap.m.Wizard`
- **Use when:** Guiding users through a long or unfamiliar multi-step sequential process where each step must be completed before advancing — onboarding, guided configuration, complex object creation. Requires 3–8 steps. Can also be deployed inside a modal dialog when the wizard is closely connected to the triggering page and users need to return quickly.
- **Do NOT use when:** 2 or fewer steps → use a Dialog with Next/Back buttons; more than 8 steps — restructure the task instead; steps are independent and can be completed in any order → use `sap.uxap.ObjectPageLayout` with sections; the task is routine or familiar to the user.
- **Rules:**
  - Minimum 3, maximum 8 steps — violating this range logs a runtime error.
  - For branching flows: set `enableBranching: true` and call `WizardStep#setNextStep()` programmatically; changing `subsequentSteps` associations at runtime is **not supported**.
  - Dynamic step insertion at runtime is **not supported** — define all `sap.m.WizardStep` instances in the `steps` aggregation upfront.
  - The final "Next Step" button label changes to `"Review"` when the last step is reached, leading users to a summary screen before final submission — do not override `finishButtonText` without reason.
  - A Cancel button is always present in the footer — it must trigger a data loss warning if fields have been modified.
  - For long forms include a "Save as Draft" option in the footer toolbar.
  - Place inside `sap.m.Page` with `enableScrolling: false` — the Wizard manages its own internal scroll.
  - When inside `sap.f.DynamicPage`: set `DynamicPage.fitContent = true` and add the `sapUiNoContentPadding` CSS class to the DynamicPage.
  - Use `renderMode: Page` for mobile-optimized rendering that shows one step at a time without scroll.
- **Gotchas:** Setting `currentStep` to step C in a path A→B→C→D activates A, B, and C — D remains unvisited. When a Wizard inside a Dialog is closed and reopened, DOM `scrollTop` resets to 0 and the Wizard automatically resets the navigator back to the first step — call `setCurrentStep()` before reopening if you need to restore the user's previous position.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants