Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent unnecessary execution of the displayValue callback in the ComboboxInput component #3048

Merged
merged 3 commits into from Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@headlessui-react/CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Forward `disabled` state to hidden inputs in form-like components ([#3004](https://github.com/tailwindlabs/headlessui/pull/3004))
- Prefer incoming `data-*` attributes, over the ones set by Headless UI ([#3035](https://github.com/tailwindlabs/headlessui/pull/3035))
- Respect `selectedIndex` for controlled `<Tab/>` components ([#3037](https://github.com/tailwindlabs/headlessui/pull/3037))
- Prevent unnecessary execution of the `displayValue` callback in the `ComboboxInput` component ([#3048](https://github.com/tailwindlabs/headlessui/pull/3048))

### Changed

Expand Down
Expand Up @@ -489,6 +489,53 @@ describe('Rendering', () => {
})
)

it(
'should keep the defaultValue when the Combobox state changes',
suppressConsoleLogs(async () => {
let data = [
{ id: 1, name: 'alice', label: 'Alice' },
{ id: 2, name: 'bob', label: 'Bob' },
{ id: 3, name: 'charlie', label: 'Charlie' },
]

function Example() {
let [person, setPerson] = useState(data[1])

return (
<Combobox value={person} onChange={setPerson} name="assignee" by="id">
<Combobox.Input displayValue={() => String(Math.random())} />
<Combobox.Button />
<Combobox.Options>
{data.map((person) => (
<Combobox.Option key={person.id} value={person}>
{person.label}
</Combobox.Option>
))}
</Combobox.Options>
</Combobox>
)
}

render(<Example />)

let value = getComboboxInput()?.value

// Toggle the state a few times combobox
await click(getComboboxButton())
await click(getComboboxButton())
await click(getComboboxButton())

// Verify the value is still the same
expect(getComboboxInput()?.value).toBe(value)

// Choose an option, which should update the value
await click(getComboboxOptions()[2])

// Verify the value changed
expect(getComboboxInput()?.value).not.toBe(value)
})
)

it(
'should close the Combobox when the input is blurred',
suppressConsoleLogs(async () => {
Expand Down
Expand Up @@ -1005,15 +1005,15 @@ function InputFn<
// which should always result in a string (since we are filling in the value of the text input),
// you don't have to use this at all, a more common UI is a "tag" based UI, which you can render
// yourself using the selected option(s).
let currentDisplayValue = (function () {
let currentDisplayValue = useMemo(() => {
if (typeof displayValue === 'function' && data.value !== undefined) {
return displayValue(data.value as unknown as TType) ?? ''
} else if (typeof data.value === 'string') {
return data.value
} else {
return ''
}
})()
}, [data.value, displayValue])

// Syncing the input value has some rules attached to it to guarantee a smooth and expected user
// experience:
Expand Down
42 changes: 42 additions & 0 deletions packages/@headlessui-vue/src/components/combobox/combobox.test.ts
Expand Up @@ -483,6 +483,48 @@ describe('Rendering', () => {
)
})

it(
'should not crash when a defaultValue is not given',
suppressConsoleLogs(async () => {
let data = [
{ id: 1, name: 'alice', label: 'Alice' },
{ id: 2, name: 'bob', label: 'Bob' },
{ id: 3, name: 'charlie', label: 'Charlie' },
]

renderTemplate({
template: html`
<Combobox v-model="person" name="assignee" by="id">
<ComboboxInput :displayValue="displayValue" />
<ComboboxButton />
<ComboboxOptions>
<ComboboxOption v-for="person in data" :key="person.id" :value="person">
{{ person.label }}
</ComboboxOption>
<ComboboxOptions>
</Combobox>
`,
setup: () => ({ person: ref(data[0]), data, displayValue: () => String(Math.random()) }),
})

let value = getComboboxInput()?.value

// Toggle the state a few times combobox
await click(getComboboxButton())
await click(getComboboxButton())
await click(getComboboxButton())

// Verify the value is still the same
expect(getComboboxInput()?.value).toBe(value)

// Choose an option, which should update the value
await click(getComboboxOptions()[1])

// Verify the value changed
expect(getComboboxInput()?.value).not.toBe(value)
})
)

it(
'should not crash when a defaultValue is not given',
suppressConsoleLogs(async () => {
Expand Down