-
Notifications
You must be signed in to change notification settings - Fork 34
fix: ensure menu-button's menu width fits the largest menu item #3109
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| import { ComponentFixture, TestBed } from "@angular/core/testing"; | ||
| import { GoabMenuButton } from "./menu-button"; | ||
| import { GoabMenuAction } from "./menu-action"; | ||
| import { Component, CUSTOM_ELEMENTS_SCHEMA } from "@angular/core"; | ||
| import { GoabButtonType, GoabIconType } from "@abgov/ui-components-common"; | ||
| import { By } from "@angular/platform-browser"; | ||
| import { fireEvent } from "@testing-library/dom"; | ||
|
|
||
| @Component({ | ||
| template: ` | ||
| <goab-menu-button | ||
| [text]="text" | ||
| [type]="type" | ||
| [leadingIcon]="leadingIcon" | ||
| [testId]="testId" | ||
| (onAction)="onAction($event)"> | ||
| <goab-menu-action | ||
| text="Action 1" | ||
| action="action1" | ||
| icon="person-circle"> | ||
| </goab-menu-action> | ||
| <goab-menu-action | ||
| text="Action 2" | ||
| action="action2" | ||
| icon="notifications"> | ||
| </goab-menu-action> | ||
| </goab-menu-button> | ||
| ` | ||
| }) | ||
| class TestMenuButtonComponent { | ||
| text?: string; | ||
| type?: GoabButtonType; | ||
| leadingIcon?: GoabIconType; | ||
| testId?: string; | ||
|
|
||
| onAction(event: unknown) { | ||
| /* do nothing */ | ||
| } | ||
| } | ||
|
|
||
| describe("GoabMenuButton", () => { | ||
| let fixture: ComponentFixture<TestMenuButtonComponent>; | ||
| let component: TestMenuButtonComponent; | ||
|
|
||
| beforeEach(async () => { | ||
| await TestBed.configureTestingModule({ | ||
| imports: [GoabMenuButton, GoabMenuAction], | ||
| schemas: [CUSTOM_ELEMENTS_SCHEMA], | ||
| declarations: [TestMenuButtonComponent] | ||
| }).compileComponents(); | ||
|
|
||
| fixture = TestBed.createComponent(TestMenuButtonComponent); | ||
| component = fixture.componentInstance; | ||
| component.text = "Menu actions"; | ||
| component.type = "primary"; | ||
| component.leadingIcon = "alarm"; | ||
| component.testId = "test-menu-button"; | ||
| fixture.detectChanges(); | ||
| }); | ||
|
|
||
| it("should render the properties", () => { | ||
| const menuButtonElement = fixture.debugElement.query(By.css("goa-menu-button")).nativeElement; | ||
| expect(menuButtonElement.getAttribute("text")).toBe("Menu actions"); | ||
| expect(menuButtonElement.getAttribute("type")).toBe("primary"); | ||
| expect(menuButtonElement.getAttribute("leading-icon")).toBe("alarm"); | ||
| expect(menuButtonElement.getAttribute("testid")).toBe("test-menu-button"); | ||
| }); | ||
|
|
||
| it("should render with leading icon", () => { | ||
| const menuButtonElement = fixture.debugElement.query(By.css("goa-menu-button")).nativeElement; | ||
| expect(menuButtonElement.getAttribute("leading-icon")).toBe("alarm"); | ||
| }); | ||
|
|
||
| it("should render without leading icon when not provided", () => { | ||
| component.leadingIcon = undefined; | ||
| fixture.detectChanges(); | ||
|
|
||
| const menuButtonElement = fixture.debugElement.query(By.css("goa-menu-button")).nativeElement; | ||
| expect(menuButtonElement.getAttribute("leading-icon")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should respond to action event", () => { | ||
| const onAction = jest.spyOn(component, "onAction"); | ||
| const menuButtonElement = fixture.debugElement.query(By.css("goa-menu-button")).nativeElement; | ||
|
|
||
| const mockDetail = { action: "action1", text: "Action 1" }; | ||
| fireEvent(menuButtonElement, new CustomEvent("_action", { detail: mockDetail })); | ||
|
|
||
| expect(onAction).toHaveBeenCalledWith(mockDetail); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,6 +55,7 @@ | |
| padding: 0 var(--goa-button-padding-lr); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| gap: var(--goa-button-gap); | ||
| align-items: center; /* for leading and trailing icon vertical alignment */ | ||
| white-space: nowrap; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of this, long menu items don't wrap, and the Popover itself doesn't respect the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also because of this, the Popover can expand to infinite width potentially, as there's nothing that can be used to control its width, so it just grows infinitely to fit the content. |
||
|
|
||
| width: 100%; | ||
| background: none; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| <svelte:options | ||
| customElement={{ | ||
| <svelte:options customElement={{ | ||
vanessatran-ddi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| tag: "goa-menu-button", | ||
| props: { | ||
| leadingIcon: { attribute: "leading-icon", type: "String" }, | ||
| } | ||
| }} | ||
| /> | ||
|
|
||
|
|
@@ -15,6 +17,7 @@ | |
| export let text: string; | ||
| export let type: "primary" | "secondary" | "tertiary" = "primary"; | ||
| export let testid: string = ""; | ||
| export let leadingIcon: GoAIconType | undefined = undefined; | ||
| // Private props | ||
|
|
@@ -26,6 +29,8 @@ | |
| let _buttonIndex = 0; | ||
| let _targetEl: HTMLElement; | ||
| // width of the menu button which is the min-width of the popover menu | ||
| let _menuWidth: number = 0; | ||
| // Reactive | ||
|
|
@@ -131,12 +136,14 @@ | |
| on:_open={open} | ||
| padded="false" | ||
| tabindex="-1" | ||
| maxwidth="none" | ||
| prevent-scroll-into-view={true} | ||
| > | ||
| <goa-button | ||
| bind:this={_targetEl} | ||
| data-testid={testid} | ||
| slot="target" | ||
| leadingicon={leadingIcon} | ||
| {type} | ||
| trailingicon={_icon} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The acceptance criteria on the story asks that |
||
| > | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,7 +34,7 @@ | |
| export let testid: string = "popover"; | ||
| export let position: "above" | "below" | "auto" = "auto"; | ||
| export let maxwidth: string = "320px"; | ||
| export let maxwidth: string | "none" = "320px"; | ||
| export let minwidth: string = ""; | ||
| export let width: string = ""; | ||
| export let height: "full" | "wrap-content" = "wrap-content"; | ||
|
|
@@ -360,7 +360,8 @@ | |
| targetRect.left > popoverRect.width; | ||
| if (rightAligned) { | ||
| _popoverEl.style.left = `${targetRect.x - (popoverRect.width - targetRect.width)}px`; | ||
| _popoverEl.style.right = "0"; | ||
| _popoverEl.style.left = ""; | ||
| } | ||
| } | ||
| </script> | ||
|
|
@@ -395,7 +396,12 @@ | |
| <slot name="target" /> | ||
| </button> | ||
|
|
||
| <div style={style("display", _open ? "block" : "none")}> | ||
| <div | ||
| style={styles( | ||
| style("display", _open ? "block" : "none"), | ||
| style("position", "relative"), | ||
| )} | ||
| > | ||
| <section | ||
| bind:clientHeight={_sectionHeight} | ||
| bind:this={_popoverEl} | ||
|
|
@@ -404,7 +410,7 @@ | |
| style={styles( | ||
| style("width", width), | ||
| style("min-width", minwidth), | ||
| style("max-width", width ? `max(${width}, ${maxwidth})` : maxwidth), | ||
| style("max-width", maxwidth === "none" ? "" : maxwidth), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| style("padding", _padded ? "var(--goa-space-m)" : "0"), | ||
| )} | ||
| > | ||
|
|
@@ -427,7 +433,6 @@ | |
| display: inline; | ||
| align-items: center; | ||
| height: 100%; | ||
| position: relative; | ||
| } | ||
| .popover-target { | ||
|
|
||




There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test failed because after angular 20 upgrade, testbed likes to have everything
standalone.After this change it will pass the test:
