operational-ui: TopBarSelect types are broken

Hi 👋🏻 Can I take this one too ?

@march213 consider following code (standard React)

function App() {
  const [state, setState] = React.useState('mango');
  const onChange = React.useCallback((event) => setState(event.target.value), [])

  return (
    <select value={state} onChange={onChange}>
      <option value="grapefruit">Grapefruit</option>
      <option value="lime">Lime</option>
      <option value="coconut">Coconut</option>
      <option value="mango">Mango</option>
    </select>
  );
}

React standard components have to deal with event, because they are building blocks. But in our case we can simplify API and onChange can be called with value directly instead of event. (Does it make sense so far?)

So we should be able to write something like this

const displayItems = [{ label: "Table", icon: TableIcon }, { label: "Bar Chart", icon: BarChartIcon }];
const [activeDisplay, setActiveDisplay] = React.useState("Table");

<TopbarSelect
  label="Display:"
  selected={activeDisplay}
  items={displayItems}
  onChange={setActiveDisplay}
/>

But our type system says that onChange can be called with ReactNode as well. Does it even make sense to have ReactNode provided to onChange? 🤔 How we gonna use ReactNode in state? (I mean generally we can, but we should not unless there is a specific reason for it).

So my idea is that onChange type signature should be (label: string) => void

This sometimes happens https://www.youtube.com/watch?v=_UZFI-8D5uA

@ImogenF Thank you !

@march213 Go for it!

Yes you can.

Please do not change the type signature of IContextMenuItem. This is used widely internally and ~could be~ is a significant breaking change.

Further, sometimes the labels of these items (even in TopbarSelect) are not directly equal to the value. Imagine a list where you want to show user-readable labels but have machine-readable values. In this case, you’d want to pass newItem.value to onChange instead of its label.

For these reasons, I think passing just a label is a little naïve compared to passing the entire item and letting the user deal with it however they want.

For consistency across our app, I propose the following interface for TopbarSelect:

onClick: ContextMenuProps["onClick"]

This should make things more compatible.

Thank you 😊 It makes perfect sense. I will simplify type signature.

But our type system says that onChange can be called with ReactNode as well. Does it even make sense to have ReactNode provided to onChange? 🤔 How we gonna use ReactNode in state? (I mean generally we can, but we should not unless there is a specific reason for it).

I was confusing about using ReactNode, but I wasn’t sure about use cases.