Skip to content

Comments

docs(si-menu): improve and extend menu example#1559

Open
robertwilde wants to merge 2 commits intosiemens:mainfrom
robertwilde:docs/improve-si-menu-example
Open

docs(si-menu): improve and extend menu example#1559
robertwilde wants to merge 2 commits intosiemens:mainfrom
robertwilde:docs/improve-si-menu-example

Conversation

@robertwilde
Copy link

@robertwilde robertwilde commented Feb 20, 2026

Closes #984

In the basic example, introduce 3 columns to trigger the menu, and simplify the menu.
Add a new "full-options" example, inline using the declarative API, and with a menu button click using the factory API.


@robertwilde robertwilde requested review from a team as code owners February 20, 2026 11:47
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves the documentation for the si-menu component by simplifying the basic example and adding a new, more comprehensive "full options" example. The changes are well-structured and enhance the documentation. All identified areas for improvement, including a high-severity accessibility issue, code cleanup opportunities, and a suggestion to improve test code readability, remain valid as they do not contradict any provided rules.

<button
type="button"
class="btn btn-circle btn-tertiary element-options-vertical"
[cdkMenuTriggerFor]="contextMenu"
Copy link
Contributor

Choose a reason for hiding this comment

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

high

This icon-only button is missing an accessible name, which is an accessibility issue. Add an aria-label attribute to describe its purpose for screen reader users, for example aria-label="Options".

        [cdkMenuTriggerFor]="contextMenu"
        aria-label="Options"

Comment on lines 18 to 29
const example1 = 'si-menu/si-menu-full-options';
test(example1, async ({ page, si }) => {
await si.visitExample(example1);
await si.runVisualAndA11yTests();
});

const example2 = 'si-menu/si-menu-bar';
test(example2, async ({ page, si }) => {
await si.visitExample(example2);
await page.getByText('Item 1').click();
await si.runVisualAndA11yTests();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For better readability and maintainability, it's good practice to use more descriptive variable names for test examples instead of generic names like example1 and example2.

Suggested change
const example1 = 'si-menu/si-menu-full-options';
test(example1, async ({ page, si }) => {
await si.visitExample(example1);
await si.runVisualAndA11yTests();
});
const example2 = 'si-menu/si-menu-bar';
test(example2, async ({ page, si }) => {
await si.visitExample(example2);
await page.getByText('Item 1').click();
await si.runVisualAndA11yTests();
});
const fullOptionsExample = 'si-menu/si-menu-full-options';
test(fullOptionsExample, async ({ page, si }) => {
await si.visitExample(fullOptionsExample);
await si.runVisualAndA11yTests();
});
const menuBarExample = 'si-menu/si-menu-bar';
test(menuBarExample, async ({ page, si }) => {
await si.visitExample(menuBarExample);
await page.getByText('Item 1').click();
await si.runVisualAndA11yTests();
});

@@ -0,0 +1,66 @@
<!-- <ng-template #contextMenu> -->
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This commented-out <ng-template> tag, along with its closing tag on line 36, appears to be a remnant from another example. Since this menu is displayed inline and not as a context menu, these comments are misleading and should be removed to improve clarity.


@Component({
selector: 'app-sample',
imports: [SiMenuModule, CdkContextMenuTrigger, CdkMenuTrigger],
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The CdkContextMenuTrigger is imported but not used in the template si-menu-full-options.html. It can be removed from the imports array to keep the dependencies clean.

Suggested change
imports: [SiMenuModule, CdkContextMenuTrigger, CdkMenuTrigger],
imports: [SiMenuModule, CdkMenuTrigger],

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.

si-menu: Improve and extend our examples

1 participant