From 282d676d1e268408d0ee1262c3d485de5ae78549 Mon Sep 17 00:00:00 2001 From: Goutham Ponnada Date: Wed, 15 Jan 2025 15:29:53 -0800 Subject: [PATCH] fix: code review suggestions --- .../ebay-accordion/accordion.stories.ts | 4 +-- .../ebay-accordion/component-browser.ts | 7 +--- src/components/ebay-accordion/index.marko | 4 +-- src/components/ebay-accordion/marko-tag.json | 2 +- .../ebay-accordion/test/mock/index.js | 2 +- .../ebay-accordion/test/test.browser.js | 32 ++++++------------- 6 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/components/ebay-accordion/accordion.stories.ts b/src/components/ebay-accordion/accordion.stories.ts index 2c2acbbdb..9568d5e30 100644 --- a/src/components/ebay-accordion/accordion.stories.ts +++ b/src/components/ebay-accordion/accordion.stories.ts @@ -53,7 +53,7 @@ export default { }, }, }, - items: { + item: { name: "@item", description: "Represents an element to be used as part of the group. Allowed attributes are `open`, `as`, `text` and `renderBody`", @@ -63,7 +63,7 @@ export default { }, onToggle: { action: "on-toggle", - description: "Triggered on toggle", + description: "Triggered on toggle of details to control auto-collapse", table: { category: "Events", defaultValue: { diff --git a/src/components/ebay-accordion/component-browser.ts b/src/components/ebay-accordion/component-browser.ts index 4a0bf2335..128a0452a 100644 --- a/src/components/ebay-accordion/component-browser.ts +++ b/src/components/ebay-accordion/component-browser.ts @@ -6,7 +6,7 @@ export interface AccordionInput extends Omit, `on${string}`> { size?: "regular" | "large"; "auto-collapse"?: AttrBoolean; "a11y-role-description"?: AttrString; - items?: Marko.AttrTag< + item?: Marko.AttrTag< Omit >; "on-toggle"?: (event: { originalEvent: Event; open: boolean }) => void; @@ -27,11 +27,6 @@ class Accordion extends Marko.Component { } }); } - - this.emit("toggle", { - originalEvent: event.originalEvent, - open: (event.originalEvent.target as HTMLDetailsElement).open, - }); } } diff --git a/src/components/ebay-accordion/index.marko b/src/components/ebay-accordion/index.marko index 6c62fee7f..46a63eb6c 100644 --- a/src/components/ebay-accordion/index.marko +++ b/src/components/ebay-accordion/index.marko @@ -5,7 +5,7 @@ static function noop() {} $ const { class: inputClass, size, - items, + item: items = [], autoCollapse, a11yRoleDescription, ...htmlInput @@ -19,7 +19,7 @@ $ (input as any).toJSON = noop; aria-roledescription=(a11yRoleDescription ?? "accordion") key="accordion-root" > - +
  • []": { + "@item []": { "attribute-groups": ["html-attributes"], "@*": { "targetProperty": null, diff --git a/src/components/ebay-accordion/test/mock/index.js b/src/components/ebay-accordion/test/mock/index.js index dca9edfdb..fa4f6e359 100644 --- a/src/components/ebay-accordion/test/mock/index.js +++ b/src/components/ebay-accordion/test/mock/index.js @@ -2,7 +2,7 @@ import { createRenderBody } from "../../../../common/test-utils/shared"; export const Default_Accordion = { - items: [ + item: [ { text: "Item 1", renderBody: createRenderBody( diff --git a/src/components/ebay-accordion/test/test.browser.js b/src/components/ebay-accordion/test/test.browser.js index df944071f..2c6d957da 100644 --- a/src/components/ebay-accordion/test/test.browser.js +++ b/src/components/ebay-accordion/test/test.browser.js @@ -25,7 +25,7 @@ describe("given the accordion in the default state", () => { }); it("should render all sections collapsed", () => { - input.items.forEach((item) => { + input.item.forEach((item) => { expect( component.getByText(item.text).closest("details").open, ).to.equal(false); @@ -34,27 +34,16 @@ describe("given the accordion in the default state", () => { describe("when first section toggled", () => { beforeEach(async () => { - await fireEvent.click(component.getByText(input.items[0].text)); - }); - - it("should emit the toggle event", async () => { - const toggleEvent = component.emitted("toggle"); - expect(toggleEvent.length).to.be.greaterThan(0); - - const [eventArg] = toggleEvent.pop(); - expect(eventArg).has.property("open"); - expect(eventArg) - .has.property("originalEvent") - .is.an.instanceOf(Event); + await fireEvent.click(component.getByText(input.item[0].text)); }); it("should open the clicked section", async () => { - const firstSection = component.getByText(input.items[0].text); + const firstSection = component.getByText(input.item[0].text); expect(firstSection.closest("details").open).to.equal(true); }); it("should close an open section when clicked again", async () => { - const firstSection = component.getByText(input.items[0].text); + const firstSection = component.getByText(input.item[0].text); expect(firstSection.closest("details").open).to.equal(true); await fireEvent.click(firstSection); expect(firstSection.closest("details").open).to.equal(false); @@ -71,23 +60,20 @@ describe("given the accordion with autocollapse enabled", () => { it("should collapse previous section when new section is opened", async () => { // Open first section - await fireEvent.click(component.getByText(input.items[0].text)); + await fireEvent.click(component.getByText(input.item[0].text)); expect( - component.getByText(input.items[0].text).closest("details").open, + component.getByText(input.item[0].text).closest("details").open, ).to.equal(true); // Open second section - await fireEvent.click(component.getByText(input.items[1].text)); + await fireEvent.click(component.getByText(input.item[1].text)); // Verify first section closed and second section opened expect( - component.getByText(input.items[0].text).closest("details").open, + component.getByText(input.item[0].text).closest("details").open, ).to.equal(false); expect( - component.getByText(input.items[1].text).closest("details").open, + component.getByText(input.item[1].text).closest("details").open, ).to.equal(true); - - const toggleEvents = component.emitted("toggle"); - expect(toggleEvents.length).to.equal(2); }); });