Does slotProps.content.children need to be written as a child of <slots.content>? Shouldn't the props spread also spread children?

haha you're right, I was messing around with the render of slots.content and forgot to change it back 😅

I would suggest that you could use the root children as the children for the content slot. It's what is done for MenuItem and makes for a clearer API, especially if customers want extra dom markup in the content.

<ComboButton icon={<Icon />}>
  <span>Some custom content <strong>that is bold</strong></span>
</ComboButton>

Which is a lot better than

<ComboButton icon={<Icon />} content={<span>Some custom content <strong>that is bold</strong></span>} />

The styling is a valid issue since to style the content slot only users would need to:

<ComboButton icon={<Icon />} content={{ className: styles.content }}>
  <span>Some custom content <strong>that is bold</strong></span>
</ComboButton>

In MenuItem we went with the assumption that this styling would not be that common and the trade off was worth the API clarity

code in MenuItem for reference:

@ling1726 the content slot is the primary slot for the ComboButton control, so this would already be the way it would be used:

<ComboButton icon={<Icon />}>
  <span>Some custom content <strong>that is bold</strong></span>
</ComboButton>

ComboButton is a bit different than MenuItem in that any non-style props (e.g. aria-* attributes, disabled, readonly, etc.) should go on the content slot's button, which is the element with role="combobox". The root slot is a <div> for styling/layout, so having root as the primary slot and just putting its children in the content slot doesn't make as much sense here.

In response to your question about whether this should live outside of the ComboBox package my feeling is that we should keep it as an internal implementation detail of ComboBox for now. AFAIK we don't have any list controls for v9 yet and no one is working on any so we don't really know how we will handle selection in those cases. Keeping this private to ComboBox lets us solve the selection issue for this control while leaving us free to change the solution later to a more general selection implementation that works for all lists if necessary while avoiding adding/deprecating public APIs.

In working on the SpinButton API I was told we are trying to avoid using TS enums in favor or string unions. Should this be changed?

ah yeah, I should update this to a string union 👍

I think you want to use forEach in this case rather than map since you don't need to the array that map produces.

Based on the guards used below (key ? ...) it looks like props.fluentKey can be undefined which means id might end up as ${idBase}-undefined.

Instead of using id, keys or something better I suggest to make value a required prop.

• value should be unique
• value is an attribute on <option>
• value is a string

Unfortunately, since value is used as a way to define the string used in the actual <input>/<button> trigger element, it doesn't need to be unique and can't be a replacement for id or key.

An example of this is a combobox that has several pinned options at the top whose values are the same as the unpinned options in the full list (seen this in a Microsoft product in the past).

Another example is someone using options as, essentially, actions like "select all", "deselect", or whatever custom thing they're making. The value for each of those would likely be an empty string.

An example of this is a combobox that has several pinned options at the top whose values are the same as the unpinned options in the full list (seen this in a Microsoft product in the past).

Do you I correctly understand that it's something like that (a snippet below)?

function App() {
  return (
    <Combobox>
      <PinnedOptions>
        <Option value="us">US</Option>
        <Option value="canada">Canada</Option>
      </PinnedOptions>
      <AllOptions>
        <Option value="us">US</Option>
        <Option value="canada">Canada</Option>
        <Option value="cz">Czech Republic</Option>
        <Option value="de">Germany</Option>
      </AllOptions>
    </Combobox>
  );
}

• React keys are not needed there as it's defined in JSX
  • We don't really need React keys in this snippet
• Let's assume that ids will be autogenerated on every Option so they are also uniq
  • They can be used for aria-activedescendant
• I expect that values are not unique in this scope as if an option is selected in PinnedOptions it also should be selected in AllOptions

Is there sense in it?

Yup, that's essentially the pinned options example I was thinking of.

I wonder if we should also clone this child so that all children returned from this function are clones? Might be surprising to have some clones and some original children.

+1 to eliminating the need for itemKey!

+1 to eliminating the need for itemKey on options.

Is the intention for the Listbox component that it can be used separately from Combobox ? It looks like a lot of the interactions and handlers are quite similar to the parent Combobox

Yup, it can be used separately 👍

Are these box shadows available in our theme tokens ? if they are now I would check with design to see why because the hard coded values are not theme aware.

#fff is definitely not theme aware, and you should use a design token for this

@smhigley can you please provide context on this?

This was discussed in other threads, but these slots are not aligned with the implementation of other components. Can you please create/link issue to follow up on this?

Here it is, also created a checkbox for it in the convergence epic: #22331

@smhigley can we please link an issue to refactor this part and avoid cloning for the reasons mentioned in the comment?

This looks like a problem 💥

I am not sure how selectedOptions are generated, but they come from state/props, right?

As selectedOptions is an array React will trigger additional re-renders every time when a reference changes. This is probably one of the reasons why I have seen so many renders.

It comes from a useControllableState hook in useSelection. There was a separate issue causing the multiple option re-renders you commented on earlier, which should've been fixed earlier. If you're still seeing multiple unnecessary renders somewhere, let me know & I'll open an issue for it