ComboboxControl and FormTokenField: enhance components with custom render callback for options

[manzoorwanijk]

What?

This PR

• Adds an optional render callback to the SuggestionsList and ComboboxControl components in order to allow the consumer to control how an option is rendered inside SuggestionsList for a combobox or any other consumer of SuggestionsList
• Adds a corresponding story to ComboboxControl to demonstrate the new behavior

Why?

In many use cases of combobox, we need to show more detail to the user for an option in the list apart from just the label. More details help users identify the correct option from the list when there are plenty of them available with similar labels.

How?

• It first adds an optional (thus full backward compatible) prop renderSuggestion to SuggestionsList component.
• If renderSuggestion is passed, it will be given preference and the suggestion will be passed to it to allow it to render the suggestion.
• It also adds renderOption prop to ComboboxControl which in turn gets passed to SuggestionsList above.

Testing Instructions

• Checkout the branch
• Run npm run storybook:dev
• Check combobox story in Storybook
• Confirm that the existing story behaves as before i.e. it is backward compatible
• Confirm that With Render Option story shows more details in the options for combobox like Age, Country.

Screenshots or screencast

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1202011579628273

[ciampo]

Hey @manzoorwanijk , thank you for the contribution!

The code changes are clear, and I definitely understand the need for this change.

The only reservation that I currently have is around API design. We're currently in the process of investigating how to best refactor CustomSelectControl using ariakit, and we'll very likely apply the same treatment to ComboboxControl and related. Therefore, I'm going to take some additional time to understand if introducing the renderSuggestion prop is compatible with those potential changes.

Apart from that, here's a few additional notes from this first round of review:

• Should we add the same prop to the FormTokenField components?
• We should add documentation about this prop to the ComboboxControl README (and potentially FormTokenField, in case)
• We should add a CHANGELOG entry

[manzoorwanijk]

• Should we add the same prop to the FormTokenField components?
• We should add documentation about this prop to the ComboboxControl README (and potentially FormTokenField, in case)
• We should add a CHANGELOG entry

@ciampo Thank you for the feedback. I agree with your suggestions. I have made the changes accordingly.

[rcanepa]

LGTM now.

I don't see any regression anymore, and the new renderOption prop works as expected.

[ciampo]

I would request that we wait before merging, as explained in an earlier comment.

Since the changes in this PR would be making changes to the public APIs of a few components, we'd like to make sure that these changes are compatible with the future changes that we are planning for these exact components, as introducing breaking changes is something that we try to avoid as much as possible for the @wordpress/components package.

Does the proposed changes have a high priority ? I'm asking because I'm quite busy these days and I need to juggle priorities in my queues.

[manzoorwanijk]

I would request that we wait before merging, as explained in an earlier comment.

I am not sure what it's supposed to look like in the future.

Does the proposed changes have a high priority?

Yes, we have a need for this change in order to use the component inside wp-admin for Jetpack.

[ciampo]

I am not sure what it's supposed to look like in the future.

That's ok, it's the job of the components squad (mainly @mirka and myself) to understand that.

Yes, we have a need for this change in order to use the component inside wp-admin for Jetpack.

I'll try to prioritize this task, then. Thank you!

[ciampo]

Sorry for the delay in answering, and thank you for your patience!

TL;DR: let's rename renderProp to __experimentalRenderItem on both components. I believe @mirka also had some further feedback to leave, so let's wait for that too.

---

More details:

After discussing the matter with @mirka , we analysed the main options that we have in terms of API surface for CustomSelectControl (and, similarly, ComboboxControl and ForkTokenField). While it's still early for us to commit "officially", we decided to explore the last approach highlighted in this comment:

• it is the most similar to the current approach, and therefore the least disruptive
• since we don't want to fully commit yet, let's add an __experimental prefix in front (as we continue the work on CustomSelectControl, we will either stabilize these props, or remove them in favor of a different approach)
• in order to have the highest flexibility, we may want to use two separate props (depending on the component):
  • one for rendering each option in the dropdown (called __experimentalRenderItem)
  • another one for rendering the selected value(s) that are shown also when the component is "closed" (called __experimentalRenderValue)
• as we stabilize these props, we may need to make some changes / expand what the options prop is capable of (e.g accept option groups, how to pass custom additional data), which may result into potential breaking changes (and therefore I would recommend that you follow the work on these components closely to avoid any disruption)

---

Question: in the current state of this PR, would you expect to be able to find the items by searching for their country / age as well ? The potential need for matching the search string against all data fields (and not just the value)

[manzoorwanijk]

TL;DR: let's rename renderProp to __experimentalRenderItem on both components. I believe @mirka also had some further feedback to leave, so let's wait for that too.

Thank you for the feedback @ciampo . I have made the changes and used __experimentalRenderItem for all the components involved to make the usage consistent.

in order to have the highest flexibility, we may want to use two separate props (depending on the component):

• one for rendering each option in the dropdown (called __experimentalRenderItem)
• another one for rendering the selected value(s) that are shown also when the component is "closed" (called __experimentalRenderValue)

I am not sure if __experimentalRenderValue will work as expected because it will affect the current value of the input being entered by user while searching for the options.

Question: in the current state of this PR, would you expect to be able to find the items by searching for their country / age as well ? The potential need for matching the search string against all data fields (and not just the value)

No, that's not currently possible. Maybe in some future iteration, we can do that.

[mirka]

Thanks for the quick turnaround! The changes are good on my end. Once Marco gives the final 👍 we're good to go.

[ciampo]

LGTM 🚀 Feel free to merge once the few remaining pieces of feedback are addressed!

I am not sure if __experimentalRenderValue will work as expected because it will affect the current value of the input being entered by user while searching for the options.

I though what we could still use the results of __experimentalRenderValue while the search input is not focused, but that would required completely replacing the input element with a custom component showing the selected value(s).

Let's leave it for now, and re-evaluate once we work on the overhaul of these components. Another thing that I'd like to do is to "normalize" the shape of options (for ComboboxControl) and suggestions (for FormTokenField) to have the same "type", and to make it clean that, whatever that type it, it's going to be the same type that __experimentalRenderItem and __experimentalRenderValue expect

Question: in the current state of this PR, would you expect to be able to find the items by searching for their country / age as well ? The potential need for matching the search string against all data fields (and not just the value)

No, that's not currently possible. Maybe in some future iteration, we can do that.

Apologies in case my question got misunderstood — I was asking mostly to understand your expectations as a user of the component. I'm not expecting that this PR introduces that functionality — we're still at the phase where we're evaluating what data should be "searchable" to the user. It's not trivial since, with the introduction of the render props, the component doesn't really know what data is shown to the user (and therefore what the user may deem "searchable")

[manzoorwanijk]

I was asking mostly to understand your expectations as a user of the component.

Yes, in the future, it would be great to have the values being searchable by anything that is rendered in the suggestions list.

[manzoorwanijk]

Feel free to merge once the few remaining pieces of feedback are addressed!

Thank you for the feedback @ciampo. I have addressed all the feedback. Feel free to merge it when ready. I am not authorized to merge PRs here.