feat(factories): use children in shorthands as render props

[layershifter]

⏩ Migration

We decided to go with soft migration, existing API with render callback will be deprecated and will create warnings.

Case 0: slot replacement

Used when you need to replace a component in a slot, for example replace Button with SplitButton in Dialog.

Before

<Dialog
  confirmButton={render =>
    render({ content: "Confirm" }, (Component, props) => {
      const { primary, ...rest } = props;

      return (
        <SplitButton
          menu={["New group message", "New channel message"]}
          button={rest}
          primary={primary}
        />
      );
    })
  }
  trigger={<Button content="Open a dialog" />}
/>

After

<Dialog
  confirmButton={{
    content: "Confirm",
    children: (Component, props) => {
      const { primary, ...rest } = props;

      return (
        <SplitButton
          menu={["New group message", "New channel message"]}
          button={rest}
          primary={primary}
        />
      );
    }
  }}
  trigger={<Button content="Open a dialog" />}
/>

Case 1: wrapping slot

Used when you need to wrap component slot with Popup or Tooltip, for example.

Before

<Button
  content={render =>
    render({ content: "Foo", tabIndex: 1 }, (Component, props) => (
      <Popup trigger={<Component {...props} />} />
    ))
  }
/>

After

<Button
  content={{
    content: "Foo",
    tabIndex: 1,
    children: (Component, props) => <Popup trigger={<Component {...props} />} />
  }}
/>

Case 2: wrapping item

Used when you need to wrap the whole item with Popup or Tooltip, for example.

Before

const items = [
  render =>
    render({ content: 'Foo' }, (Component, props) => <Popup trigger={<Component {...props} />} />),
  { content: 'Bar' },
]

<Menu items={items} />

After

const items = [
  {
    content: "Foo",
    children: (Component, props) => <Popup trigger={<Component {...props} />} />
  },
  { content: "Bar" }
];

const MenuExample = () => <Menu items={items} />;

Case 3: delayed execution

Is used in async scenarios. We deprecated this use case, see Motivation part. There is only one way to migrate is to lift up state, it works even now.

CodeSandbox with Before & After: https://codesandbox.io/s/stardust-ui-example-23icy

Before

<Avatar
  image="https://stardust-ui.github.io/react/public/images/avatar/small/matt.jpg"
  status={render => (
    <QueryMock>
      {status => {
        if (status) {
          return render({
            color: "green",
            icon: "stardust-checkmark",
            title: "Available"
          });
        }

        return render({
          color: "grey",
          icon: "sync",
          title: "Loading"
        });
      }}
    </QueryMock>
  )}
/>

After

<QueryMock>
  {status => (
    <Avatar
      image="https://stardust-ui.github.io/react/public/images/avatar/small/matt.jpg"
      status={
        status
          ? {
              color: "green",
              icon: "stardust-checkmark",
              title: "Available"
            }
          : {
              color: "grey",
              icon: "sync",
              title: "Loading"
            }
      }
    />
  )}
</QueryMock>

⌚️ Motivation

Existing API with render callback is very powerful and in the same time is over-complicated: it's hard to explain to people how it works and it's hard to use for frequent use cases.

<Menu
  items={[
    (
      render // callback 1
    ) =>
      render(
        // callback 2
        {
          key: "editorials",
          content: "Editorials"
        },
        (Component, props) => (
          <Popup content="Foo" trigger={<Component {...props} />} />
        )
      )
  ]}
/>

Other cons

• it allows developers to do really strange things with passing render down into React tree:

<Menu
  items={[
    render => <ItemRenderer render={render} />} />
  ]}
/>

• the resolution of kind prop is broken:

<Menu
  items={[
    render =>
      render(
        {
          key: "editorials",
          kind: "divider",
          content: "Editorials"
        },
        (Component, props) => <Component /> // renders MenuItem 😭
      )
  ]}
/>

⭐️ Solution

React has well known render props pattern and I suggest to use instead as more friendly option.

I propose to have only one type of usage, via children prop:

• it allows to deprecate render callback, otherwise there will be not obvious collision if we will decide to go with deprecation
• single option is better than multiple

<Menu
  items={[
    {
      key: "events",
      content: "Upcoming Events",
      children: (Component, props) => (
        <Popup content="Popup" trigger={<Component {...props} />} />
      )
    }
  ]}
/>

It still allows to pass shorthand props.

[jurokapsiar]

this looks awesome! Migration would be a bit tricky for the consumers though, let'see how we can handle that.

[jurokapsiar]

passing the render down the tree, even though strange, was used for a reason - if the shorthand props are resolved asynchronously. Do you see a good solution in this case, or should the recommendation only be to move the state up and resolve everything before rendering?

[layershifter]

Do you see a good solution in this case, or should the recommendation only be to move the state up and resolve everything before rendering?

Lift up state, with hooks it will be easier. It also will solve issues like re-positioning requirement.

[codecov]

Codecov Report

Merging #1951 into master will increase coverage by 0.02%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1951      +/-   ##
==========================================
+ Coverage   75.83%   75.85%   +0.02%     
==========================================
  Files         160      160              
  Lines        5569     5571       +2     
  Branches     1611     1630      +19     
==========================================
+ Hits         4223     4226       +3     
+ Misses       1332     1331       -1     
  Partials       14       14

Impacted Files	Coverage Δ
packages/react/src/types.ts	50% <ø> (ø)	⬆️
...ackages/react/src/components/Dropdown/Dropdown.tsx	87.81% <100%> (ø)	⬆️
packages/react/src/lib/factories.ts	94.66% <100%> (+0.3%)	⬆️
...t/src/components/Dropdown/DropdownSelectedItem.tsx	75% <75%> (+1.47%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f15857...502c55c. Read the comment docs.

[layershifter]

@miroslavstastny I am not sure about this typing, any thoughts?

[miroslavstastny]

I like it 😃