fix: code review suggestions

eBay · Jan 23, 2025 · 282d676 · 282d676
1 parent 93b00ad
commit 282d676
Show file tree

Hide file tree

Showing 6 changed files with 16 additions and 35 deletions.
diff --git 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 <ebay-details/> 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
@@ -6,7 +6,7 @@ export interface AccordionInput extends Omit<Marko.Input<"ul">, `on${string}`> {
     size?: "regular" | "large";
     "auto-collapse"?: AttrBoolean;
     "a11y-role-description"?: AttrString;
-    items?: Marko.AttrTag<
+    item?: Marko.AttrTag<
         Omit<DetailsInput, "size" | "alignment" | "layout" | `on${string}`>
     >;
     "on-toggle"?: (event: { originalEvent: Event; open: boolean }) => void;
@@ -27,11 +27,6 @@ class Accordion extends Marko.Component<Input> {
                 } 
             });            
         }
-
-        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
@@ -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"
 >
-    <for|item, i| of=items || []>
+    <for|item| of=items || []>
         <li>
             <ebay-details
                 ...item

diff --git a/src/components/ebay-accordion/marko-tag.json b/src/components/ebay-accordion/marko-tag.json
@@ -11,7 +11,7 @@
     "@as": "string",
     "@auto-collapse": "boolean",
     "@a11y-role-description": "string",
-    "@items <item>[]": {
+    "@item <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
@@ -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
@@ -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);
     });
 });