ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

SelectDropdown makes two onChange calls on each change #202

Closed jorgefilipecosta closed 3 years ago

jorgefilipecosta commented 3 years ago

Each time we change an option the SelectDropdown makes two one change calls instead of just one.

I think it happens because of this code: https://github.com/ItsJonQ/g2/blob/eda852261c10dcf192f2c93366e17f2844d0d7d2/packages/components/src/select-dropdown/use-select-dropdown.js#L73-L89

I guess we are calling onchange with commitValue and value?

ItsJonQ commented 3 years ago

Looking into this now!

ItsJonQ commented 3 years ago

Got it! It looks like undesired double onChange happened with the isPreviewable feature was introduced.

For context, the useEffect + subscribe combo is used to synchronize data/actions between substate and incoming component props.

For the isPreviewable, we need to sync both value and commitValue (as you noted @jorgefilipecosta ). We only want to do that for isPreviewable.

The bug happens for non-previewable SelectDropdown setups, as there's no logic to prevent the preview-only onChange from firing.

Adding this solves it

    // Propogate (preview) value
    useEffect(() => {
        if (!isPreviewable) return; // <-- here!
        return store.subscribe(
            (value) => onChange({ selectedItem: value }),
            (state) => state.value,
            shallowCompare,
        );
    }, [onChange, store, isPreviewable]);

In the GIF demo (below), I have the SelectDropdown hooking into the onChange callback and logging the results.

Screen Capture on 2021-01-04 at 13-29-34

With the change (above), it only fires once now 🙏