chore(TDOPS-5724): remove react-bootstrap from faceted-search (#5042)

* chore(TDOPS-5724): remove react-bootstrap from faceted-search * add changeset * some fixes around popover preventDefault * better styling and accessibility with button * fix snapshots --------- Co-authored-by: Sebastien LE MOUILLOUR <[email protected]>
Talend · Dec 7, 2023 · a7b06bc · a7b06bc
1 parent b0ab239
commit a7b06bc
Show file tree

Hide file tree

Showing 27 changed files with 387 additions and 264 deletions.
diff --git a/.changeset/clean-files-shake.md b/.changeset/clean-files-shake.md
@@ -0,0 +1,5 @@
+---
+'@talend/design-system': patch
+---
+
+Fix DS Popover controlled state to allow disclosure props binding
diff --git a/.changeset/fifty-teachers-add.md b/.changeset/fifty-teachers-add.md
@@ -0,0 +1,5 @@
+---
+'@talend/react-faceted-search': major
+---
+
+Remove @talend/react-bootstrap from Faceted Search package to use DS Popover with controlled state
diff --git a/packages/design-system/src/components/Popover/Popover.cy.tsx b/packages/design-system/src/components/Popover/Popover.cy.tsx
@@ -1,7 +1,9 @@
 /* eslint-disable testing-library/prefer-screen-queries */
 
 /* eslint-disable testing-library/await-async-queries */
-import { Popover, ButtonPrimary, CollapsiblePanel } from '../../';
+import { useState } from 'react';
+
+import { ButtonPrimary, CollapsiblePanel, Popover } from '../../';
 
 context('<Popover />', () => {
 	describe('default', () => {
@@ -11,7 +13,7 @@ context('<Popover />', () => {
 					data-testid="my.popover"
 					disclosure={<ButtonPrimary data-testid="my.button">Open popover</ButtonPrimary>}
 				>
-					Popover content
+					<p>Popover content</p>
 				</Popover>,
 			);
 
@@ -21,19 +23,16 @@ context('<Popover />', () => {
 			cy.findByTestId('my.popover').should('be.visible');
 		});
 
-		it('should prevent default', () => {
+		it('should prevent default without controlled state', () => {
 			cy.mount(
 				<CollapsiblePanel
 					title="panel"
 					metadata={[
 						<Popover
 							key="my.popover"
+							placement="left"
 							data-testid="my.popover"
-							disclosure={
-								<ButtonPrimary onClick={() => {}} data-testid="my.button">
-									Open popover
-								</ButtonPrimary>
-							}
+							disclosure={<ButtonPrimary data-testid="my.button">Open popover</ButtonPrimary>}
 						>
 							<p>Popover content</p>
 						</Popover>,
@@ -47,6 +46,51 @@ context('<Popover />', () => {
 			cy.findByTestId('panel.section').should('not.exist');
 			cy.findByTestId('my.button').click();
 			cy.findByTestId('panel.section').should('not.exist');
+			cy.findByTestId('my.popover').should('be.visible');
+		});
+
+		it('should prevent default with controlled state', () => {
+			const PopoverWithControlledState = () => {
+				const [open, setOpen] = useState(false);
+				return (
+					<CollapsiblePanel
+						title="panel"
+						metadata={[
+							<Popover
+								key="my.popover"
+								data-testid="my.popover"
+								placement="left"
+								open={open}
+								onOpenChange={setOpen}
+								disclosure={
+									<ButtonPrimary
+										onClick={event => {
+											event.preventDefault();
+											event.stopPropagation();
+											setOpen(!open);
+										}}
+										data-testid="my.button"
+									>
+										Open popover
+									</ButtonPrimary>
+								}
+							>
+								<p>Popover content</p>
+							</Popover>,
+						]}
+					>
+						Some text
+					</CollapsiblePanel>
+				);
+			};
+
+			cy.mount(<PopoverWithControlledState />);
+
+			// Disclosure onClick should stop propagation and not open the CollapsiblePanel container
+			cy.findByTestId('panel.section').should('not.exist');
+			cy.findByTestId('my.button').click();
+			cy.findByTestId('panel.section').should('not.exist');
+			cy.findByTestId('my.popover').should('be.visible');
 		});
 	});
 });
diff --git a/packages/design-system/src/components/Popover/Popover.tsx b/packages/design-system/src/components/Popover/Popover.tsx
@@ -1,12 +1,12 @@
-import { useRef, Fragment } from 'react';
-import type { ReactNode, MouseEvent } from 'react';
+import React, { Fragment, useRef } from 'react';
+import type { MouseEvent, ReactNode } from 'react';
 
-import { Placement, FloatingArrow, FloatingPortal } from '@floating-ui/react';
+import { FloatingArrow, FloatingPortal, Placement } from '@floating-ui/react';
 import classNames from 'classnames';
 
 import tokens from '@talend/design-tokens';
 
-import { renderOrClone, ChildOrGenerator } from '../../renderOrClone';
+import { renderOrClone } from '../../renderOrClone';
 import { usePopover } from './usePopover';
 
 import theme from './Popover.module.scss';
@@ -22,7 +22,7 @@ type PopoverOptions = {
 };
 
 export type PopoverProps = {
-	disclosure: ChildOrGenerator<ReactNode, object>;
+	disclosure: ReactNode;
 	children: ReactNode | ((props: any) => ReactNode);
 } & PopoverOptions;
 
@@ -48,7 +48,11 @@ export function Popover({
 		e.preventDefault();
 		e.stopPropagation();
 	};
-	const childrenProps = popover.getReferenceProps({ onClick });
+
+	let childrenProps = popover.getReferenceProps({ onClick });
+	if (disclosure && React.isValidElement(disclosure)) {
+		childrenProps = popover.getReferenceProps({ onClick, ...disclosure.props });
+	}
 
 	return (
 		<>

diff --git a/packages/design-system/src/components/Popover/usePopover.tsx b/packages/design-system/src/components/Popover/usePopover.tsx
@@ -1,17 +1,18 @@
-import { useState, useMemo } from 'react';
+import { useMemo, useState } from 'react';
 import type { MutableRefObject } from 'react';
+
 import {
-	useFloating,
 	arrow,
 	autoUpdate,
-	offset,
 	flip,
+	offset,
+	Placement,
 	shift,
 	useClick,
 	useDismiss,
-	useRole,
+	useFloating,
 	useInteractions,
-	Placement,
+	useRole,
 } from '@floating-ui/react';
 
 const ARROW_HEIGHT = 7;
@@ -88,9 +89,7 @@ export function usePopover({
 
 	const context = data.context;
 
-	const click = useClick(context, {
-		enabled: controlledOpen == null,
-	});
+	const click = useClick(context);
 	const dismiss = useDismiss(context);
 	const role = useRole(context);
 

diff --git a/packages/faceted-search/package.json b/packages/faceted-search/package.json
@@ -36,7 +36,6 @@
     "@talend/bootstrap-theme": "^8.3.1",
     "@talend/daikon-tql-client": "^1.3.1",
     "@talend/utils": "^2.8.0",
-    "@talend/react-bootstrap": "^2.2.1",
     "@talend/design-tokens": "^2.10.1",
     "classnames": "^2.3.2",
     "date-fns": "^1.30.1",

diff --git a/packages/faceted-search/src/components/AdvancedSearch/AdvancedSearch.component.js b/packages/faceted-search/src/components/AdvancedSearch/AdvancedSearch.component.js
@@ -4,7 +4,6 @@ import { useState } from 'react';
 import PropTypes from 'prop-types';
 
 import { ButtonIcon, Icon } from '@talend/design-system';
-import { FormControl } from '@talend/react-bootstrap';
 import { getTheme } from '@talend/react-components/lib/theme';
 
 import { USAGE_TRACKING_TAGS } from '../../constants';
@@ -71,9 +70,9 @@ export function AdvancedSearch({
 	const advSearchId = `${id}-adv-search`;
 	return (
 		<div id={advSearchId} className={css('adv-search')}>
-			<form id={`${advSearchId}-form`} onSubmit={formSubmit}>
+			<form id={`${advSearchId}-form`} role="search" onSubmit={formSubmit}>
 				<Icon name="talend-filter" size="M" className={css('adv-search-filter-icon')} />
-				<FormControl
+				<input
 					id={`${id}-form`}
 					name="advanced-search-faceted"
 					type="search"
@@ -83,7 +82,7 @@ export function AdvancedSearch({
 					className={css('adv-search-input', { 'has-error': error })}
 					aria-label={placeholder || t('ADV_SEARCH_FACETED_ARIA', 'Advanced Faceted Search')}
 					autoFocus
-					role="search"
+					role="searchbox"
 					onKeyDown={onKeyDownHandler}
 					onChange={onChangeHandler}
 				/>

diff --git a/packages/faceted-search/src/components/AdvancedSearch/AdvancedSearch.component.test.js b/packages/faceted-search/src/components/AdvancedSearch/AdvancedSearch.component.test.js
@@ -28,7 +28,7 @@ describe('AdvancedSearch', () => {
 			</FacetedManager>,
 		);
 		// then
-		expect(screen.getByRole('search')).toHaveValue(initialQuery);
+		expect(screen.getByRole('searchbox')).toHaveValue(initialQuery);
 	});
 	it('should update the query when input change', () => {
 		// given
@@ -39,9 +39,9 @@ describe('AdvancedSearch', () => {
 				<AdvancedSearch onSubmit={onSubmit} />
 			</FacetedManager>,
 		);
-		fireEvent.change(screen.getByRole('search'), { target: { value: query } });
+		fireEvent.change(screen.getByRole('searchbox'), { target: { value: query } });
 		// then
-		expect(screen.getByRole('search')).toHaveValue(query);
+		expect(screen.getByRole('searchbox')).toHaveValue(query);
 	});
 	it('should call the onChange props when input change', () => {
 		// given
@@ -53,7 +53,7 @@ describe('AdvancedSearch', () => {
 				<AdvancedSearch onChange={onChange} onSubmit={onSubmit} />
 			</FacetedManager>,
 		);
-		fireEvent.change(screen.getByRole('search'), { target: { value: query } });
+		fireEvent.change(screen.getByRole('searchbox'), { target: { value: query } });
 		// then
 		expect(onChange).toHaveBeenCalled();
 		expect(onChange.mock.calls.length).toBe(1);
@@ -67,7 +67,7 @@ describe('AdvancedSearch', () => {
 				<AdvancedSearch onSubmit={onSubmit} />
 			</FacetedManager>,
 		);
-		fireEvent.keyDown(screen.getByRole('search'), { key: 'Enter' });
+		fireEvent.keyDown(screen.getByRole('searchbox'), { key: 'Enter' });
 		// then
 		expect(onSubmit).toHaveBeenCalled();
 		expect(onSubmit.mock.calls.length).toBe(1);
@@ -81,7 +81,7 @@ describe('AdvancedSearch', () => {
 				<AdvancedSearch onKeyDown={onKeyDown} onSubmit={onSubmit} />
 			</FacetedManager>,
 		);
-		fireEvent.keyDown(screen.getByRole('search'), { key: 'Enter' });
+		fireEvent.keyDown(screen.getByRole('searchbox'), { key: 'Enter' });
 		// then
 		expect(onKeyDown).toHaveBeenCalled();
 		expect(onKeyDown.mock.calls.length).toBe(1);

diff --git a/...-search/src/components/AdvancedSearch/__snapshots__/AdvancedSearch.component.test.js.snap b/...-search/src/components/AdvancedSearch/__snapshots__/AdvancedSearch.component.test.js.snap
@@ -7,6 +7,7 @@ exports[`AdvancedSearch should render by default 1`] = `
 >
   <form
     id="some-id-adv-search-form"
+    role="search"
   >
     <svg
       aria-hidden="true"
@@ -19,11 +20,11 @@ exports[`AdvancedSearch should render by default 1`] = `
     <input
       aria-label="Advanced Faceted Search"
       autocomplete="off"
-      class="adv-search-input theme-adv-search-input form-control"
+      class="adv-search-input theme-adv-search-input"
       id="some-id-form"
       name="advanced-search-faceted"
       placeholder="Enter your query"
-      role="search"
+      role="searchbox"
       type="search"
       value=""
     />

diff --git a/...es/faceted-search/src/components/Badges/BadgeCheckboxes/BadgeCheckboxes.component.test.js b/...es/faceted-search/src/components/Badges/BadgeCheckboxes/BadgeCheckboxes.component.test.js
@@ -1,8 +1,8 @@
-import { render } from '@testing-library/react';
-import { BadgeFacetedProvider } from '../../context/badgeFaceted.context';
+import { render, within } from '@testing-library/react';
 
-import { BadgeCheckboxes } from './BadgeCheckboxes.component';
 import getDefaultT from '../../../translate';
+import { BadgeFacetedProvider } from '../../context/badgeFaceted.context';
+import { BadgeCheckboxes } from './BadgeCheckboxes.component';
 
 const t = getDefaultT();
 
@@ -37,7 +37,8 @@ describe('BadgeCheckboxes', () => {
 		render(<BadgeWithContext {...props} />);
 		// Then
 
-		expect(document.querySelectorAll('span')[2]).toHaveTextContent('All');
+		const badge = document.querySelector('#tc-badge-select-myId-badge-checkboxes');
+		expect(within(badge).getByText('All')).toBeVisible();
 	});
 	it('should return "All" when value is empty', () => {
 		// Given
@@ -52,7 +53,8 @@ describe('BadgeCheckboxes', () => {
 		// When
 		render(<BadgeWithContext {...props} />);
 		// Then
-		expect(document.querySelectorAll('span')[2]).toHaveTextContent('All');
+		const badge = document.querySelector('#tc-badge-select-myId-badge-checkboxes');
+		expect(within(badge).getByText('All')).toBeVisible();
 	});
 	it('should return the amount of values when values are equal or greater than 4', () => {
 		// Given
@@ -73,7 +75,8 @@ describe('BadgeCheckboxes', () => {
 		// When
 		render(<BadgeWithContext {...props} />);
 		// Then
-		expect(document.querySelectorAll('span')[2]).toHaveTextContent('5 value');
+		const badge = document.querySelector('#tc-badge-select-myId-badge-checkboxes');
+		expect(within(badge).getByText('5 values')).toBeVisible();
 	});
 	it('should return only the checked values', () => {
 		// Given
@@ -94,8 +97,9 @@ describe('BadgeCheckboxes', () => {
 		// When
 		render(<BadgeWithContext {...props} />);
 		// Then
-		expect(document.querySelectorAll('span')[2]).toHaveTextContent('one');
-		expect(document.querySelectorAll('span')[3]).toHaveTextContent('two');
-		expect(document.querySelectorAll('span')[4]).toHaveTextContent('five');
+		const badge = document.querySelector('#tc-badge-select-myId-badge-checkboxes');
+		expect(within(badge).getByText('one')).toBeVisible();
+		expect(within(badge).getByText('two')).toBeVisible();
+		expect(within(badge).getByText('five')).toBeVisible();
 	});
 });
diff --git a/packages/faceted-search/src/components/Badges/BadgeDate/BadgeDate.component.test.js b/packages/faceted-search/src/components/Badges/BadgeDate/BadgeDate.component.test.js
@@ -1,7 +1,8 @@
 import { render } from '@testing-library/react';
-import { BadgeDate } from './BadgeDate.component';
-import { BadgeFacetedProvider } from '../../context/badgeFaceted.context';
+
 import getDefaultT from '../../../translate';
+import { BadgeFacetedProvider } from '../../context/badgeFaceted.context';
+import { BadgeDate } from './BadgeDate.component';
 
 const badgeFacetedContextValue = {
 	onDeleteBadge: jest.fn(),
@@ -26,7 +27,7 @@ describe('BadgeDate', () => {
 		);
 		// Then
 		expect(container.firstChild).toMatchSnapshot();
-		expect(document.querySelector('button#myId-badge-date-action-overlay')).toHaveTextContent(
+		expect(document.querySelector('#myId-badge-date-action-overlay')).toHaveTextContent(
 			'2011-10-01',
 		);
 	});