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.

Sign up for GitHub

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

Jump to bottom

Update selectable search input state when the input changes #2776

Merged
merged 29 commits into from May 22, 2024
Merged

Update selectable search input state when the input changes #2776

merged 29 commits into from May 22, 2024

Conversation

ddouglasz
Copy link
Contributor

Summary

Update selectable search input state when the input changes

Closes #2773

@ddouglasz ddouglasz requested a review from a team as a code owner April 11, 2024 13:55
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Apr 11, 2024 •
edited

🦋 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 Vercel
Copy link

vercel bot commented Apr 11, 2024 •
edited

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@kark
Copy link
Contributor

kark commented Apr 22, 2024

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
emmenko reviewed Apr 22, 2024
View reviewed changes
Copy link
Member

@emmenko emmenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

packages/components/inputs/selectable-search-input/src/selectable-search-input.tsx Outdated
/**
* Default value of the input. Consists of text input and selected option.
*/
defaultValue: TValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

All recent feedback updated here: 3a3bb9d

@ddouglasz
Copy link
Contributor Author

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

Thank you.

@CarlosCortizasCT
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

ddouglasz commented May 21, 2024 •
edited

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

@ddouglasz ddouglasz merged commit 79318f5 into main May 22, 2024
7 checks passed
@ddouglasz ddouglasz deleted the SHIELD-1195-selectable-search-input-state-update branch May 22, 2024 09:43
@ct-changesets ct-changesets bot mentioned this pull request May 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emmenko emmenko emmenko left review comments

@CarlosCortizasCT CarlosCortizasCT CarlosCortizasCT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SelectableSearchInput state doesn't update when input changes
5 participants
@ddouglasz @CarlosCortizasCT @chloe0592 @kark @emmenko