gsoft-inc/sg-orbit

πŸ’‘<Option> component

Closed this issue Β· 2 comments

Description

Currently, the Autocomplete, Listbox, Menu and Select components all represent items using the Item component.

A typical usage of the Item component looks like this:

<Select>
    <Item key="earth">Earth</Item>
    <Item key="jupiter">Jupiter</Item>
</Select>

Problems

The Item component presents a few limitations:

Fragments

The following code would not work, because React only considers that Select has 3 children: "earth", "jupiter", and "React.Fragment".

It's not possible to get the key from within the fragment.

// ❌ Not valid
<Select>
    <Item key="earth">Earth</Item>
    <Item key="jupiter">Jupiter</Item>
    {includeMoons && (
      <>
        <Item key="moon">Moon</Item>
        <Item key="io">Io</Item>
      </>
    )}
</Select>

Turning the fragment into an array of elements, doesn't work either.

When doing so, React adds a prefix to element.key which breaks the euristics used in Orbit to parse the keys.

// ❌ Not valid
<Select>
    <Item key="earth">Earth</Item>
    <Item key="jupiter">Jupiter</Item>
    {includeMoons && [
      <Item key="moon">Moon</Item>,
      <Item key="io">Io</Item>
    ]}
</Select>

Lack of composition

Similar to the issues with fragments, Items cannot be grouped into components, as the children would become the component, instead of the Items within the component.

// ❌ Not valid
function MyItem() {
 return <Item key="earth">Earth</Item>
}

export default function() { 
  return (
    <Select>
      <Item key="jupiter">Jupiter</Item>
      <MyItem />
    </Select>
  )
} 

Not recommended by React

React's Special Props Warning specifically warns against using keys in app logic.

Keys are used as reconciling hints by React, and there is no guaranteed that implementations details will remain the same in the future.

Proposal: Option component

I propose the creation of a new Option component which solves all the previously mentionned issues.

// βœ… Valid
<Select>
    <Option value="earth">Earth</Option>
    <Option value="jupiter">Jupiter</Option>
    {includeMoons && (
      <>
        <Option value="moon">Moon</Option>
        <Option value="io">Io</Option>
      </>
     )}
</Select>

How?

Using Context and useLayoutEffect, basically. I wrote a gist for the implementation.

Usage:

// Custom component that uses the `useOptions` hook
function Select({ children }) {
  const [options, OptionsProvider] = useOptions();

  return (
    <div className="my-custom-select">
        {options.map(option => (
          <button onClick={() => console.log(option.value)}>{option.content}</button >
        )}
        
        {/* The children must be rendered. Can be hidden in CSS with display: none */}
        <OptionsProvider>
          {children}
        </OptionsProvider>
      </button >
  );
}

// User-land code that uses the custom component, and uses the <Option /> component
function App() {
  return (
    <Select>
      <Option value={1}>John</Option>
      
      {/* βœ… Fragments are supported */}
      <>
        <Option value={2}>Paul</Option>
      </>
      
      {/* βœ… Custom components are also supported */}
      <MyOptions />
    </Select>
  );
}

function MyOptions() {
  return (
    <>
      <Option value={3}>Ringo</Option>
      <Option value={4}>George</Option>
    </>
  )
}

Issues with this proposal

  • SSR warning about useLayoutEffect. Maybe use useEffect instead?
    • Note: Sligthly off-topic, but React Router v6.4 recently did a massive rewrite of their API, to pass routes as an object, instead of with React components. I assume this was done for SSR, as updating a parent from a child in React is not SSR friendly.
    • Should form elements even be server-rendered?
  • Keeping it backward compatible for a time, with the Item component, or at least making the transition from Item to Option as seamless as possible.
  • Maybe instead of creating a brand new component, we can just modify Item to have the improvements.

If you can think of anything else, feedback is welcome and appreciated! πŸ˜„

Hey @tjosepo!

Thanks for the proposal, it's very interesting. I am not sure I am willing to re-open this part of the code thought πŸ˜… Basically it's a shared collection API used by every components using items, it's not a standalone implementation for the Select component.

Composition should work. Maybe something has broken for the Select. Pretty sure at some point I had written Chromatic tests testing composition for every component using the collection API. Maybe something broke for the Select component and I removed the feature at some point since nobody seemed to have use this in like 2 years πŸ˜†

Anyway, you can find a working Chromatic test showcasing composition with an ActiveOption component in the Listbox Chromatic test suite.

There's also always the option to embed your own logic. In the following test case it's simply mapping an array but pretty much everything is possible as the clients has full control on the rendered items.

i do agree that this is a better way of doing this.

Moved into a discussion for now, since realistically we won’t be tackling this short term