Update selectable search input state when the input changes

[ddouglasz]

Summary

Update selectable search input state when the input changes

Closes #2773

[changeset-bot]

🦋 Changeset detected

Latest commit: 12d6909

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 96 packages

Name	Type
@commercetools-uikit/selectable-search-input	Minor
@commercetools-uikit/inputs	Minor
@commercetools-frontend/ui-kit	Minor
@commercetools-uikit/design-system	Minor
@commercetools-uikit/calendar-time-utils	Minor
@commercetools-uikit/calendar-utils	Minor
@commercetools-uikit/hooks	Minor
@commercetools-uikit/i18n	Minor
@commercetools-uikit/localized-utils	Minor
@commercetools-uikit/utils	Minor
@commercetools-uikit/accessible-hidden	Minor
@commercetools-uikit/avatar	Minor
@commercetools-uikit/card	Minor
@commercetools-uikit/collapsible-motion	Minor
@commercetools-uikit/collapsible-panel	Minor
@commercetools-uikit/collapsible	Minor
@commercetools-uikit/constraints	Minor
@commercetools-uikit/data-table-manager	Minor
@commercetools-uikit/data-table	Minor
@commercetools-uikit/field-errors	Minor
@commercetools-uikit/field-label	Minor
@commercetools-uikit/field-warnings	Minor
@commercetools-uikit/grid	Minor
@commercetools-uikit/icons	Minor
@commercetools-uikit/label	Minor
@commercetools-uikit/link	Minor
@commercetools-uikit/loading-spinner	Minor
@commercetools-uikit/messages	Minor
@commercetools-uikit/notifications	Minor
@commercetools-uikit/pagination	Minor
@commercetools-uikit/primary-action-dropdown	Minor
@commercetools-uikit/progress-bar	Minor
@commercetools-uikit/stamp	Minor
@commercetools-uikit/tag	Minor
@commercetools-uikit/text	Minor
@commercetools-uikit/tooltip	Minor
@commercetools-uikit/view-switcher	Minor
@commercetools-uikit/accessible-button	Minor
@commercetools-uikit/flat-button	Minor
@commercetools-uikit/icon-button	Minor
@commercetools-uikit/link-button	Minor
@commercetools-uikit/primary-button	Minor
@commercetools-uikit/secondary-button	Minor
@commercetools-uikit/secondary-icon-button	Minor
@commercetools-uikit/dropdown-menu	Minor
@commercetools-uikit/async-creatable-select-field	Minor
@commercetools-uikit/async-select-field	Minor
@commercetools-uikit/creatable-select-field	Minor
@commercetools-uikit/date-field	Minor
@commercetools-uikit/date-range-field	Minor
@commercetools-uikit/date-time-field	Minor
@commercetools-uikit/localized-multiline-text-field	Minor
@commercetools-uikit/localized-text-field	Minor
@commercetools-uikit/money-field	Minor
@commercetools-uikit/multiline-text-field	Minor
@commercetools-uikit/number-field	Minor
@commercetools-uikit/password-field	Minor
@commercetools-uikit/radio-field	Minor
@commercetools-uikit/search-select-field	Minor
@commercetools-uikit/select-field	Minor
@commercetools-uikit/text-field	Minor
@commercetools-uikit/time-field	Minor
@commercetools-uikit/async-creatable-select-input	Minor
@commercetools-uikit/async-select-input	Minor
@commercetools-uikit/checkbox-input	Minor
@commercetools-uikit/creatable-select-input	Minor
@commercetools-uikit/date-input	Minor
@commercetools-uikit/date-range-input	Minor
@commercetools-uikit/date-time-input	Minor
@commercetools-uikit/input-utils	Minor
@commercetools-uikit/localized-money-input	Minor
@commercetools-uikit/localized-multiline-text-input	Minor
@commercetools-uikit/localized-rich-text-input	Minor
@commercetools-uikit/localized-text-input	Minor
@commercetools-uikit/money-input	Minor
@commercetools-uikit/multiline-text-input	Minor
@commercetools-uikit/number-input	Minor
@commercetools-uikit/password-input	Minor
@commercetools-uikit/radio-input	Minor
@commercetools-uikit/rich-text-input	Minor
@commercetools-uikit/rich-text-utils	Minor
@commercetools-uikit/search-select-input	Minor
@commercetools-uikit/search-text-input	Minor
@commercetools-uikit/select-input	Minor
@commercetools-uikit/select-utils	Minor
@commercetools-uikit/text-input	Minor
@commercetools-uikit/time-input	Minor
@commercetools-uikit/toggle-input	Minor
@commercetools-uikit/spacings-inline	Minor
@commercetools-uikit/spacings-inset-squish	Minor
@commercetools-uikit/spacings-inset	Minor
@commercetools-uikit/spacings-stack	Minor
@commercetools-uikit/buttons	Minor
@commercetools-uikit/fields	Minor
@commercetools-uikit/spacings	Minor
visual-testing-app	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
ui-kit	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 21, 2024 4:01pm

[CarlosCortizasCT]

To be honest, I have mixed feelings with this.

The value property is currently used more like a default value to be used when initializing the component than the value itself as this component is more an uncontrolled one but it seems the requirement is to be able to control its state from the outside (controlled component).

I think we're mixing both strategies here.

My proposal would be to have both value and defaultValue properties and make the component be able to work in the two modes.

What do others think about it? @commercetools/shield-team-fe

[ddouglasz]

My proposal would be to have both value and defaultValue properties and make the component be able to work in the two modes.

updated here: be21255
Thank you

[CarlosCortizasCT]

My proposal would be to have both value and defaultValue properties and make the component be able to work in the two modes.

updated here: be21255 Thank you

Thanks @ddouglasz

However, I still would like to read the rest of the team point of view about this.

cc/ @commercetools/shield-team-fe

[ddouglasz]

However, I still would like to read the rest of the team point of view about this.
cc/ commercetools/shield-team-fe

That makes sense and this may likely be a breaking change.
@kark @chloe0592 @emmenko Please what would you suggest?

[chloe0592]

I agree with the proposed changes. Introducing both value and defaultValue would offer flexibility for both controlled and uncontrolled states. 👍

[kark]

Thanks for checking with the team.

My proposal would be to have both value and defaultValue properties and make the component be able to work in the two modes.

This proposal makes sense to me and this approach is justified in my opinion

[emmenko]

From my side, I'm also good with having the two props. Thanks for the proposal / fix!

[emmenko]

Is this supposed to be a required field?

If yes, it should probably be considered a breaking change.
If not, it should be handled as an optional field.

[ddouglasz]

Oh true, seems to me like it should be optional, assuming we do not expect users to always provide a default value.
The initial implementation already sets it to an empty string in the absence of a default value.

[CarlosCortizasCT]

The problem I see is that we currently only have the value property but we're actually using it as default value.
If we now change the behaviour of the already exposed value property to drive the internal state and add the new defaultValue, all current consumers will be affected so hence this would be a breaking change as Nicola mentioned.

We need to double think if we can somehow come up with a solution that can avoid the breaking change.

[CarlosCortizasCT]

Nicola and I had a conversation about this and we agreed releasing a major ui-kit version just for this change currently is not convenient.
Although the long term solution is having both a defaultValue and a value properties to handle both controlled and uncontrolled states, we think it could benefit us to hold that change until we have a better reason to release a breaking change version.

What we can do in the meantime is to introduce a new temporary property (example: experimentalValue) that can be used for the controlled version of the component. Then, we will have the two properties that we need, but with the "wrong" names and at least we will have a escape hatch for those (very few) use cases we know so far that require the controlled version of the component.

Once we get to the point when it will be convenient to release an ui-kit major version, we will rename those properties and probably publish a codemod which consumers can run in their codebase, to migrate from one names to the others automatically.

[ddouglasz]

All recent feedback updated here: 3a3bb9d

[ddouglasz]

@CarlosCortizasCT PR updated based on our last discussion here : 5d1edea

Thank you.

[CarlosCortizasCT]

I think we still need to update the Storybook story. As discussed last week, currently it's implemented as if it's controlling the state of the component where it is not.

Also, now that we've introduced a new function called handleDropdownChange, then one we have to handle the input change has a potentially confusing name (handleChange).
I suggest we update its name to use the same pattern we use for other callbacks we pass to the input (handleTextInputFocus and handleTextInputBlur) so for instance it could be something like handleTextInputChange.

[CarlosCortizasCT]

I also was wondering if the last change we've introduced with managing the dropdown option could be a breaking change for current consumers.
I think we should check some examples to make sure.

[ddouglasz]

I also was wondering if the last change we've introduced with managing the dropdown option could be a breaking change for current consumers. I think we should check some examples to make sure.

Great point 💯
Let me do some findings on this. If that is the case, we may need to go back to the initial implementation since we are trying to avoid a breaking change for now.

[ddouglasz]

I think we still need to update the Storybook story. As discussed last week, currently it's implemented as if it's controlling the state of the component where it is not.

...I suggest we update its name to use the same pattern we use for other callbacks we pass to the input (handleTextInputFocus and handleTextInputBlur) so for instance it could be something like handleTextInputChange

updated here: c0e73a3
Thank you