Skip to content

Commit

Permalink
fix(Select): enforce content update after render (#156)
Browse files Browse the repository at this point in the history
* fix(Select): enforce content update after render

* style(Select): format

* refactor(Select): request content update only if the renderer is provided

* style(Select): format

* test(Select): add more tests

* fix(Select): watch children prop too

* test(Select): add test for children renderer

* Update test/Select.spec.tsx

Co-authored-by: Sergey Vinogradov <[email protected]>

---------

Co-authored-by: Sergey Vinogradov <[email protected]>
  • Loading branch information
Lodin and vursen authored Nov 15, 2023
1 parent e131230 commit fe90a52
Show file tree
Hide file tree
Showing 8 changed files with 304 additions and 501 deletions.
631 changes: 182 additions & 449 deletions package-lock.json

Large diffs are not rendered by default.

7 changes: 6 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,15 @@
"devDependencies": {
"@esm-bundle/chai": "^4.3.4-fix.0",
"@testing-library/react": "^14.0.0",
"@testing-library/user-event": "^14.5.1",
"@types/chai-as-promised": "^7.1.7",
"@types/chai-dom": "^1.11.0",
"@types/karma": "^6.3.4",
"@types/karma-chrome-launcher": "^3.1.1",
"@types/karma-mocha": "^1.3.1",
"@types/mocha": "^10.0.1",
"@vitejs/plugin-react": "^4.0.0",
"chai-as-promised": "^7.1.1",
"chai-dom": "^1.11.0",
"concurrently": "^8.2.0",
"esbuild": "^0.18.2",
Expand Down Expand Up @@ -534,6 +537,8 @@
"./RichTextEditor": "./RichTextEditor.js",
"./Scroller": "./Scroller.js",
"./Select": "./Select.js",
"./SideNav": "./SideNav.js",
"./SideNavItem": "./SideNavItem.js",
"./SplitLayout": "./SplitLayout.js",
"./Tab": "./Tab.js",
"./Tabs": "./Tabs.js",
Expand Down Expand Up @@ -569,4 +574,4 @@
"./css/material/Typography.css": "./css/material/Typography.css",
"./css/material/UserColors.css": "./css/material/UserColors.css"
}
}
}
21 changes: 19 additions & 2 deletions src/Select.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,16 @@
import { type ComponentType, type ForwardedRef, forwardRef, type ReactElement, type ReactNode } from 'react';
import {
type ComponentType,
type ForwardedRef,
forwardRef,
type ReactElement,
type ReactNode,
useEffect,
useRef,
} from 'react';
import { Select as _Select, type SelectElement, type SelectProps as _SelectProps } from './generated/Select.js';
import { useSimpleOrChildrenRenderer } from './renderers/useSimpleOrChildrenRenderer.js';
import type { ReactSimpleRendererProps } from './renderers/useSimpleRenderer.js';
import useMergedRefs from './utils/useMergedRefs.js';

export * from './generated/Select.js';

Expand All @@ -14,10 +23,18 @@ export type SelectProps = Partial<Omit<_SelectProps, 'children' | 'renderer'>> &
}>;

function Select(props: SelectProps, ref: ForwardedRef<SelectElement>): ReactElement | null {
const innerRef = useRef<SelectElement>(null);
const [portals, renderer] = useSimpleOrChildrenRenderer(props.renderer, props.children);
const finalRef = useMergedRefs(innerRef, ref);

useEffect(() => {
if (props.renderer || props.children) {
innerRef.current?.requestContentUpdate();
}
}, [innerRef.current, props.renderer, props.children]);

return (
<_Select {...props} ref={ref} renderer={renderer}>
<_Select {...props} ref={finalRef} renderer={renderer}>
{portals}
</_Select>
);
Expand Down
13 changes: 13 additions & 0 deletions src/utils/useMergedRefs.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { type ForwardedRef, type RefCallback, useCallback } from 'react';

export default function useMergedRefs<T extends HTMLElement>(...refs: ReadonlyArray<ForwardedRef<T>>): RefCallback<T> {
return useCallback((element: T) => {
refs.forEach((ref) => {
if (typeof ref === 'function') {
ref(element);
} else if (!!ref) {
ref.current = element;
}
});
}, refs);
}
102 changes: 62 additions & 40 deletions test/Select.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,22 +1,18 @@
import { expect, use as useChaiPlugin } from '@esm-bundle/chai';
import { cleanup, render } from '@testing-library/react/pure.js';
import { render } from '@testing-library/react';
import userEvent, { type UserEvent } from '@testing-library/user-event';
import chaiAsPromised from 'chai-as-promised';
import chaiDom from 'chai-dom';
import type { ReactElement } from 'react';
import { ListBox } from '../src/ListBox.js';
import { Item } from '../src/Item.js';
import { Select, type SelectElement } from '../src/Select.js';
import catchRender from './utils/catchRender.js';
import createOverlayCloseCatcher from './utils/createOverlayCloseCatcher.js';
import { Select } from '../src/Select.js';
import { findByQuerySelector } from './utils/findByQuerySelector.js';

useChaiPlugin(chaiDom);
useChaiPlugin(chaiAsPromised);

describe('Select', () => {
const overlayTag = 'vaadin-select-overlay';

const [ref, catcher] = createOverlayCloseCatcher<SelectElement>(overlayTag, (ref) => {
ref.opened = false;
});

const items = [
{ label: 'Foo', value: 'foo' },
{ label: 'Bar', value: 'bar' },
Expand All @@ -31,47 +27,73 @@ describe('Select', () => {
);
}

function isListBoxRendered(node: Node) {
return node instanceof HTMLElement && node.localName.includes('list-box');
}

async function assert(container: HTMLElement) {
const select = container.querySelector('vaadin-select');
expect(select).to.exist;
async function assert(user: ReturnType<UserEvent['setup']>) {
const select = await findByQuerySelector('vaadin-select');

const valueButton = select!.querySelector('vaadin-select-value-button');
expect(valueButton).to.exist;
const valueButton = await findByQuerySelector('vaadin-select-value-button', select);
expect(valueButton).to.have.text('Bar');

valueButton!.dispatchEvent(new PointerEvent('click', { bubbles: true }));

const overlay = document.querySelector('vaadin-select-overlay');
expect(overlay).to.exist;

await catchRender(overlay!, isListBoxRendered);
await user.click(valueButton);

const overlay = await findByQuerySelector('vaadin-select-overlay');
expect(overlay).to.have.text('FooBar');
}

afterEach(cleanup);
afterEach(catcher);
let user: ReturnType<UserEvent['setup']>;

it('should use children if no renderer property set', async () => {
const { container } = render(<Select ref={ref} items={items} value="bar" />);
await assert(container);
beforeEach(() => {
user = userEvent.setup();
});

it('should use renderer prop if it is set', async () => {
const { container } = render(<Select ref={ref} items={items} renderer={Renderer} value="bar" />);
await assert(container);
it('should use items if no renderer property set', async () => {
render(<Select items={items} value="bar" />);
await assert(user);
});

it('should use children render function as a renderer prop', async () => {
const { container } = render(
<Select ref={ref} value="bar">
{Renderer}
</Select>,
);
it('should correctly render the value if default value changed', async () => {
const { rerender } = render(<Select renderer={Renderer} value="bar" />);
await expect(findByQuerySelector('vaadin-select-value-button')).to.eventually.have.text('Bar');

rerender(<Select renderer={Renderer} value="foo" />);
await expect(findByQuerySelector('vaadin-select-value-button')).to.eventually.have.text('Foo');
});

await assert(container);
describe('renderer', () => {
function NewRenderer() {
return (
<ListBox>
<Item value="foo">Foo</Item>
<Item value="bar">Bar</Item>
<Item value="baz">Baz</Item>
</ListBox>
);
}

it('should use renderer prop if it is set', async () => {
render(<Select renderer={Renderer} value="bar" />);
await assert(user);
});

it('should use children render function as a renderer prop', async () => {
render(<Select value="bar">{Renderer}</Select>);
await assert(user);
});

it('should correctly render the value if renderer prop is changed', async () => {
render(<Select renderer={Renderer} value="bar" />);
await findByQuerySelector('vaadin-select-value-button');

render(<Select renderer={NewRenderer} value="bar" />);

await expect(findByQuerySelector('vaadin-select-value-button')).to.eventually.have.text('Bar');
});

it('should correctly render the value if children prop is changed', async () => {
render(<Select value="bar">{Renderer}</Select>);
await findByQuerySelector('vaadin-select-value-button');
render(<Select value="bar">{NewRenderer}</Select>);

await expect(findByQuerySelector('vaadin-select-value-button')).to.eventually.have.text('Bar');
});
});
});
10 changes: 1 addition & 9 deletions test/kitchen-sink/Row8.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,7 @@ export default function Row8() {
</RadioButton>
</RadioGroup>
<RichTextEditor></RichTextEditor>
<Select
label="Select"
value="2"
items={[
{ label: 'One', value: '1' },
{ label: 'Two', value: '2' },
]}
renderer={SelectListBox}
/>
<Select label="Select" value="2" renderer={SelectListBox} />
</BoardRow>
);
}
5 changes: 5 additions & 0 deletions test/types.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
declare namespace Chai {
interface PromisedAssertion {
text(text: string | string[]): PromisedAssertion;
}
}
16 changes: 16 additions & 0 deletions test/utils/findByQuerySelector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { waitFor } from '@testing-library/react';

export async function findByQuerySelector<K extends keyof HTMLElementTagNameMap>(
query: K,
container?: Document | HTMLElement,
): Promise<HTMLElementTagNameMap[K]>;
export async function findByQuerySelector<E extends Element = Element>(
query: string,
container?: Document | HTMLElement,
): Promise<E>;
export async function findByQuerySelector(
query: string,
container: Document | HTMLElement = document,
): Promise<Element> {
return await waitFor(() => container.querySelector(query)!);
}

0 comments on commit fe90a52

Please sign in to comment.