Skip to content

Commit e1025dd

Browse files
committed
fix: address copilot comments
1 parent a82c501 commit e1025dd

2 files changed

Lines changed: 15 additions & 15 deletions

File tree

static/js/publisher/components/ComboBox/ComboBox.tsx

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -169,24 +169,26 @@ const ComboBox: FC<ComboBoxProps> = ({
169169
});
170170
}, [value, options]);
171171

172-
// we don't run onChange until after we actually render some real options to choose from, this
173-
// avoids running it multiple times with and empty value during init (e.g. when fetching the
174-
// options from an API)
175172
const isOnchangeDisabledRef = useRef(true);
176173

177174
// Run the onChange callback when we change the selectedItem. We don't pass the callback as a
178175
// Downshift prop because it wouldn't run when we set the selectedItem inside the state reducer
179176
useEffect(() => {
180-
if (isOnchangeDisabledRef.current) {
181-
if (options?.length > 0) {
182-
isOnchangeDisabledRef.current = false;
183-
}
184-
return;
185-
}
186-
177+
if (isOnchangeDisabledRef.current) return;
187178
onChange?.(comboBoxState.selectedItem?.value ?? null);
188179
}, [comboBoxState.selectedItem?.value]);
189180

181+
// we don't run onChange until after we actually render some real options to choose from, this
182+
// avoids running it multiple times with an empty selectedItem.value during init (e.g. when
183+
// fetching the options from an API and we're waiting for the response); it's IMPORTANT that this
184+
// effect runs after the one that triggers onChange, because we want to make sure not to trigger
185+
// it on the first render
186+
useEffect(() => {
187+
if (isOnchangeDisabledRef.current && options?.length > 0) {
188+
isOnchangeDisabledRef.current = false;
189+
}
190+
}, [options]);
191+
190192
return (
191193
<Downshift<ComboBoxItem>
192194
{...comboBoxState}
@@ -236,15 +238,13 @@ const ComboBox: FC<ComboBoxProps> = ({
236238
</div>
237239
<ul
238240
className={`p-combobox__options-panel ${comboBoxState.isOpen ? "active" : ""}`}
239-
{...getMenuProps()}
240-
>
241+
{...getMenuProps()}>
241242
{comboBoxState.isOpen &&
242243
comboBoxState.filteredOptions.map((item) => (
243244
<li
244245
{...getItemProps({ item })}
245246
key={item.value}
246-
className="p-combobox__option"
247-
>
247+
className="p-combobox__option">
248248
{item.label}
249249
</li>
250250
))}

static/js/publisher/components/ComboBox/__tests__/ComboBox.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ describe("ComboBox", () => {
115115

116116
it("listbox has correct attributes", () => {
117117
renderComponent();
118-
expect(elements.input).toHaveAttribute(
118+
expect(elements.listbox).toHaveAttribute(
119119
"aria-labelledby",
120120
elements.label.id,
121121
);

0 commit comments

Comments
 (0)