refactor(accordion): adapt native standard for behaviour (#3218)

* refactor: first try * refactor: another try * chore: update implementation for accordion * chore: update sr snapshots * fix: issue with initOpenIndex * fix: issue with typescript * fix: issue from PR * fix: issue from stencil typescript * fix: issue with accordion item * fix: issue with component test for accordion item * fix: issue with component test for vue * Update accordion-a11y.spec.ts * Update accordion-a11y.spec.ts * Update accordion-item-a11y.spec.ts * fix: issue with linting * Update accordion.scss * Update accordion-item.scss * Update accordion-item.spec.tsx --------- Co-authored-by: Nicolas Merget <[email protected]>
db-ui · Nov 19, 2024 · aebd21d · aebd21d
1 parent 5d3cb0f
commit aebd21d
Show file tree

Hide file tree

Showing 17 changed files with 167 additions and 154 deletions.
diff --git a/...apshots__/accordion/patternhub/accordion-properties-should-match-screenshot.png b/...apshots__/accordion/patternhub/accordion-properties-should-match-screenshot.png
diff --git a/package-lock.json b/package-lock.json
diff --git a/packages/components/scripts/post-build/angular.ts b/packages/components/scripts/post-build/angular.ts
@@ -217,6 +217,10 @@ export default (tmp?: boolean) => {
 			{
 				from: '} from "../../utils"',
 				to: ', enableCustomElementAttributePassing } from "../../utils"'
+			},
+			{
+				from: /this.ref.nativeElement/g,
+				to: 'this.ref?.nativeElement'
 			}
 		];
 

diff --git a/packages/components/scripts/post-build/components.ts b/packages/components/scripts/post-build/components.ts
@@ -195,15 +195,7 @@ export const getComponents = (): Component[] => [
 	},
 
 	{
-		name: 'tag',
-		overwrites: {
-			angular: [
-				{
-					from: /this.ref.nativeElement/g,
-					to: 'this.ref?.nativeElement'
-				}
-			]
-		}
+		name: 'tag'
 	},
 	{
 		name: 'checkbox',

diff --git a/packages/components/src/components/accordion-item/accordion-item.lite.tsx b/packages/components/src/components/accordion-item/accordion-item.lite.tsx
@@ -39,23 +39,25 @@ export default function DBAccordionItem(props: DBAccordionItemProps) {
 	// jscpd:ignore-end
 
 	return (
-		<details
-			ref={ref}
-			id={state._id}
-			class={cls('db-accordion-item', props.className)}
-			aria-disabled={getBooleanAsString(props.disabled)}
-			open={state._open}
-			name={props.name}>
-			<summary onClick={(event) => state.toggle(event)}>
-				<Show when={props.headlinePlain}>{props.headlinePlain}</Show>
-				<Show when={!props.headlinePlain}>
-					<Slot name="headline" />
-				</Show>
-			</summary>
-			<div>
-				<Show when={props.content}>{props.content}</Show>
-				<Show when={!props.content}>{props.children}</Show>
-			</div>
-		</details>
+		<li id={state._id} class={cls('db-accordion-item', props.className)}>
+			<details
+				aria-disabled={getBooleanAsString(props.disabled)}
+				ref={ref}
+				open={state._open}>
+				<summary onClick={(event) => state.toggle(event)}>
+					<Show when={props.headlinePlain}>
+						{props.headlinePlain}
+					</Show>
+					<Show when={!props.headlinePlain}>
+						<Slot name="headline" />
+					</Show>
+				</summary>
+				<div>
+					<Show when={props.content} else={props.children}>
+						{props.content}
+					</Show>
+				</div>
+			</details>
+		</li>
 	);
 }
diff --git a/packages/components/src/components/accordion-item/accordion-item.scss b/packages/components/src/components/accordion-item/accordion-item.scss
@@ -11,17 +11,33 @@ $db-accordion-item-border-radius: variables.$db-border-radius-sm;
 	position: relative;
 	inline-size: 100%;
 	border-radius: $db-accordion-item-border-radius;
+	list-style-type: "";
+
+	> details {
+		&[open] {
+			summary + div {
+				@media screen and (prefers-reduced-motion: no-preference) {
+					animation: accordion-open #{variables.$db-transition-straight-emotional}
+						normal both;
+				}
+			}
+
+			summary::after {
+				transform: form-components.$dropdown-icon-transform;
+			}
+		}
+
+		&[aria-disabled="true"] {
+			pointer-events: none;
+			opacity: component.$default-disabled;
+		}
+	}
 
 	summary + div {
 		padding: variables.$db-spacing-fixed-md;
 		padding-block-end: variables.$db-spacing-fixed-lg;
 	}
 
-	&[aria-disabled="true"] {
-		pointer-events: none;
-		opacity: component.$default-disabled;
-	}
-
 	summary {
 		@extend %dropdown-icon;
 		@extend %db-overwrite-font-size-md;
@@ -54,17 +70,4 @@ $db-accordion-item-border-radius: variables.$db-border-radius-sm;
 			border-radius: variables.$db-border-radius-xs;
 		}
 	}
-
-	&[open] {
-		summary + div {
-			@media screen and (prefers-reduced-motion: no-preference) {
-				animation: accordion-open #{variables.$db-transition-straight-emotional}
-					normal both;
-			}
-		}
-
-		summary::after {
-			transform: form-components.$dropdown-icon-transform;
-		}
-	}
 }
diff --git a/packages/components/src/components/accordion-item/accordion-item.spec.tsx b/packages/components/src/components/accordion-item/accordion-item.spec.tsx
@@ -24,6 +24,9 @@ const testA11y = () => {
 		await mount(comp);
 		const accessibilityScanResults = await new AxeBuilder({ page })
 			.include('.db-accordion-item')
+			// Showcase uses <li> outside of <ul> in this case
+			// TODO: Let's investigate whether we could prevent this deactivation later on
+			.disableRules(['listitem'])
 			.analyze();
 
 		expect(accessibilityScanResults.violations).toEqual([]);

diff --git a/packages/components/src/components/accordion-item/model.ts b/packages/components/src/components/accordion-item/model.ts
@@ -26,10 +26,6 @@ export type DBAccordionItemDefaultProps = {
 	 * Title of the accordion-item as plain text
 	 */
 	headlinePlain?: string;
-	/**
-	 * Set details name for exclusive accordions, see https://chromestatus.com/feature/6710427028815872
-	 */
-	name?: string;
 };
 
 export type DBAccordionItemProps = DBAccordionItemDefaultProps &

diff --git a/packages/components/src/components/accordion/accordion.lite.tsx b/packages/components/src/components/accordion/accordion.lite.tsx
@@ -9,18 +9,20 @@ import {
 } from '@builder.io/mitosis';
 import { DBAccordionItemDefaultProps } from '../accordion-item/model';
 import { DBAccordionProps, DBAccordionState } from './model';
-import { cls } from '../../utils';
+import { cls, uuid } from '../../utils';
 import DBAccordionItem from '../accordion-item/accordion-item.lite';
+import { DEFAULT_ID } from '../../shared/constants';
 
 useMetadata({});
 
 export default function DBAccordion(props: DBAccordionProps) {
-	const ref = useRef<HTMLDivElement>(null);
+	const ref = useRef<HTMLUListElement>(null);
 	// jscpd:ignore-start
 	const state = useStore<DBAccordionState>({
-		openItems: [],
-		clickedId: '',
+		_id: DEFAULT_ID,
+		_name: '',
 		initialized: false,
+		_initOpenIndexDone: false,
 		convertItems(
 			items: unknown[] | string | undefined
 		): DBAccordionItemDefaultProps[] {
@@ -35,89 +37,80 @@ export default function DBAccordion(props: DBAccordionProps) {
 			}
 
 			return [];
-		},
-		handleItemClick: (id: string) => {
-			if (state.openItems.includes(id)) {
-				if (props.behaviour === 'single') {
-					state.openItems = [];
-				} else {
-					state.openItems = state.openItems.filter(
-						(oItem) => oItem !== id
-					);
-				}
-			} else if (props.behaviour === 'single') {
-				state.openItems = [id];
-			} else {
-				state.openItems = [...state.openItems, id];
-			}
-
-			if (props.onChange) {
-				props.onChange(state.openItems);
-			}
 		}
 	});
 
 	onMount(() => {
+		state._id = props.id || 'accordion-' + uuid();
 		state.initialized = true;
+		state._initOpenIndexDone = true;
 	});
 	// jscpd:ignore-end
 
 	onUpdate(() => {
-		if (ref && state.initialized) {
-			const childDetails = ref.getElementsByTagName('details');
-			if (childDetails) {
-				let initOpenItems: string[] = [];
-				Array.from<HTMLDetailsElement>(childDetails).forEach(
-					(details: HTMLDetailsElement, index: number) => {
-						const id = details.id;
-						if (
-							details.open ||
-							props.initOpenIndex?.includes(index)
-						) {
-							initOpenItems.push(id);
-						}
-						const summaries =
-							details.getElementsByTagName('summary');
-						if (summaries?.length > 0) {
-							summaries[0].addEventListener('click', () => {
-								state.clickedId = id;
-							});
-						}
+		// If we have a single behaviour we first check for
+		// props.name otherwise for state_id
+		if (state.initialized) {
+			if (props.behaviour === 'single') {
+				if (props.name) {
+					if (state._name !== props.name) {
+						state._name = props.name;
+					}
+				} else {
+					if (state._name !== state._id && state._id) {
+						state._name = state._id;
 					}
-				);
-				if (props.behaviour === 'single' && initOpenItems.length > 1) {
-					initOpenItems = [initOpenItems[0]];
 				}
-				state.openItems = initOpenItems;
-				state.initialized = false;
+			} else {
+				state._name = '';
 			}
 		}
-	}, [ref, state.initialized]);
-
-	onUpdate(() => {
-		if (state.clickedId?.length > 0) {
-			state.handleItemClick(state.clickedId);
-			state.clickedId = '';
-		}
-	}, [state.clickedId]);
+	}, [state.initialized, props.name, props.behaviour, state._id]);
 
 	onUpdate(() => {
 		if (ref) {
 			const childDetails = ref.getElementsByTagName('details');
 			if (childDetails) {
-				Array.from<HTMLDetailsElement>(childDetails).forEach(
-					(details: HTMLDetailsElement) => {
-						details.open = state.openItems.includes(details.id);
+				for (const details of Array.from<HTMLDetailsElement>(
+					childDetails
+				)) {
+					if (state._name === '') {
+						details.removeAttribute('name');
+					} else {
+						details.name = state._name;
 					}
-				);
+				}
+			}
+		}
+	}, [ref, state._name]);
+
+	onUpdate(() => {
+		if (ref && state._initOpenIndexDone) {
+			if (props?.initOpenIndex && props.initOpenIndex?.length > 0) {
+				const childDetails = ref.getElementsByTagName('details');
+				if (childDetails) {
+					const initOpenIndex =
+						props.behaviour === 'single' &&
+						props.initOpenIndex.length > 1
+							? [props.initOpenIndex[0]] // use only one index for behaviour=single
+							: props.initOpenIndex;
+					Array.from<HTMLDetailsElement>(childDetails).forEach(
+						(details: HTMLDetailsElement, index: number) => {
+							if (initOpenIndex.includes(index)) {
+								details.open = true;
+							}
+						}
+					);
+				}
 			}
+			state._initOpenIndexDone = false;
 		}
-	}, [state.openItems]);
+	}, [ref, state._initOpenIndexDone, props.initOpenIndex]);
 
 	return (
-		<div
+		<ul
 			ref={ref}
-			id={props.id}
+			id={state._id}
 			class={cls('db-accordion', props.className)}
 			data-variant={props.variant}>
 			<Show when={!props.items}>{props.children}</Show>
@@ -133,6 +126,6 @@ export default function DBAccordion(props: DBAccordionProps) {
 					)}
 				</For>
 			</Show>
-		</div>
+		</ul>
 	);
 }
diff --git a/packages/components/src/components/accordion/accordion.scss b/packages/components/src/components/accordion/accordion.scss
@@ -15,6 +15,10 @@ The styling for the spacings between the items is defined in the accordion, wher
 The spacings are not part of the styling of the accordion items themselves.
  */
 .db-accordion {
+	padding: 0;
+	margin: 0;
+	list-style-type: "";
+
 	/*
 	default variant: prop variant is not set
 	*/

diff --git a/packages/components/src/components/accordion/accordion.spec.tsx b/packages/components/src/components/accordion/accordion.spec.tsx
@@ -30,13 +30,16 @@ const openAccordion: any = (
 
 const actionAccordion: any = (
 	<DBAccordion behaviour="single">
-		<DBAccordionItem data-testid="item1" headline="Test">
+		<DBAccordionItem data-testid="item1" headlinePlain="Test">
 			<DBButton data-testid="button">Click me</DBButton>
 		</DBAccordionItem>
-		<DBAccordionItem data-testid="item2" headline="Test 2">
+		<DBAccordionItem data-testid="item2" headlinePlain="Test 2">
 			<DBTextarea data-testid="textarea" />
 		</DBAccordionItem>
-		<DBAccordionItem disabled={true} data-testid="item3" headline="Test 3">
+		<DBAccordionItem
+			disabled={true}
+			data-testid="item3"
+			headlinePlain="Test 3">
 			<DBButton data-testid="button2">Click me</DBButton>
 		</DBAccordionItem>
 	</DBAccordion>
@@ -75,7 +78,10 @@ const testAction = () => {
 		await component.getByTestId('item2').click();
 		await expect(component.getByTestId('button')).toBeHidden();
 		await expect(component.getByTestId('textarea')).toBeVisible();
-		await expect(component.getByTestId('item3')).toBeDisabled();
+		await expect(
+			component.getByTestId('item3')
+			// VUE: .getByRole('group')
+		).toBeDisabled();
 	});
 
 	test('click inside item works', async ({ mount }) => {