[@labs/react] support multiple children with multiple slots #2985

taylor-vann · 2022-06-02T20:23:25Z

Related to #2734

This PR supports multiple children by assigning them to multiple slots by wrapping them in a <div styles="display: contents">.

A client can provide a children map similar to how we account for events.
The syntax is as follows:

const BasicElementComponent = createComponent(
  window.React,
  elementName,
  BasicElement,
  basicElementEvents,
  displayName,
  { <react_children_property_name>: <slot_name> },
);

Which results in a children config like:

const BasicElementComponent = createComponent(
  window.React,
  elementName,
  BasicElement,
  basicElementEvents,
  displayName,
  {
    headIcons: 'head-icons',
    content: 'content',
    tailIcons: 'tail-icons',
  },
);

Tests are added to verify rendered elements are rendered with the correct slot names.

changeset-bot · 2022-06-02T20:23:27Z

🦋 Changeset detected

Latest commit: 05d4b5f

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-06-02T20:23:50Z

📊 Tachometer Benchmark Results

Summary

⏱ lit-element-list
⏱ lit-html-kitchen-sink
⏱ lit-html-repeat
⏱ lit-html-template-heavy
⏱ reactive-element-list

Results

⏱ lit-element-list

Currently running in Benchmarks #3281…

⏱ lit-html-kitchen-sink

Currently running in Benchmarks #3281…

⏱ lit-html-repeat

Currently running in Benchmarks #3281…

⏱ lit-html-template-heavy

Currently running in Benchmarks #3281…

⏱ reactive-element-list

Currently running in Benchmarks #3281…

_{tachometer-reporter-action v2 for Benchmarks}

sorvell

See comments; we also need to add documentation to the readme for this.

sorvell · 2022-06-03T15:58:09Z

packages/labs/react/src/create-component.ts

+      key: slot,
+      slot,
+    },
+    children


I expected this to be an array. Why isn't it?

This is arguable and we should sync up on this before changing the code, but I think it's preferable to set slot attributes where possible rather than to create a wrapper DOM node.

However, if a wrapper node is needed, probably we should just make one and not set slots. So, this would involve iterating the children until you find a non-element node (text nodes or component references): if so making the wrapper; if not, cloneElement and set slots.

I believe React.ReactNode can be any value that react will render: string array ref node element. Children are passed through a single property name in react / jsx (or as "children"). So when a child is asked to createSlottedChild it wraps the entire prop to a slot via one wrap.

I figured this was less operations for the same behavior, choosing to wrap the entire children prop in a single function than search and create multiple. If the CSS display contents let's us avoid styling I figured this would be the most direct way to slot quickly and cheaply

A clarifying question: Are we expecting a user to pass in some value than a React.ReactNode?

Creating the wrapper DOM node itself is likely the most expensive part. If we can avoid it and just set slot attributes, we probably should. I wanted to explore what was possible via their API and so made this experiment which might be useful.

sorvell · 2022-06-03T16:11:11Z

packages/labs/react/src/create-component.ts

  React: typeof ReactModule,
  tagName: string,
  elementClass: Constructor<I>,
  events?: E,
-  displayName?: string
+  displayName?: string,
+  children?: C


I think it makes more sense to call this argument slots.

Sounds good!

sorvell · 2022-06-03T16:11:58Z

packages/labs/react/src/create-component.ts

 ) => {
  const Component = React.Component;
  const createElement = React.createElement;
+  const reactChildren = children;


Why rename this? Let's just choose a better name as suggested above.

sorvell · 2022-06-03T16:17:50Z

packages/labs/react/src/test/create-component_test.tsx

@@ -73,11 +73,18 @@ suite('createComponent', () => {
    onBar: 'bar',
  };

+  const basicElementChildren = {


Again, recommend calling this basicElementSlots

Sounds good!

sorvell · 2022-06-03T16:23:01Z

packages/labs/react/src/test/create-component_test.tsx

@@ -333,6 +340,19 @@ suite('createComponent', () => {
    assert.equal(el.firstElementChild!.localName, 'div');
  });

+  test('can set multiple children to multiple slots', async () => {
+    const children = window.React.createElement('div');
+    const foos = window.React.createElement('div');


We need to test that you can specify a a set of children for a slot. Ideally we'd be testing with JSX here as well. I think in that case it would be a JSX fragment and without JSX I guess you can make that via a React.createElement(React.Fragment, ...

Nice! Yah I'll add that test case

sorvell · 2022-06-03T16:24:52Z

packages/labs/react/src/test/create-component_test.tsx

+
+    assert.equal(el.childNodes.length, 3);
+    assert.equal(el.children?.[0].getAttribute('slot'), null);
+    assert.equal(el.children?.[1].getAttribute('slot'), 'slot-a');


These should be display: contents right? Let's test for that.

Yah let's! Sounds good

taylor-vann · 2022-06-03T17:32:39Z

Ugh dumb thumbs, how do I cancel this review ;_;

kevinpschaaf · 2022-06-03T17:37:10Z

packages/labs/react/src/create-component.ts

  React: typeof ReactModule,
  tagName: string,
  elementClass: Constructor<I>,
  events?: E,
-  displayName?: string
+  displayName?: string,
+  children?: C


Nit (naming): The fact that children is ambiguous enough that it needs to be renamed below is a smell... I'd prefer calling this slotMap or something like that; same with events -> eventMap (along with the types Children -> SlotMap and Events -> EventMap.

khanh-le-otsv · 2023-02-17T03:59:45Z

Could I ask for why this is not being merged? Thank you.
Do you need help to move this forward?

add slotted children, test slotted children

1f05630

reorder args to not break clients yet

ff83719

taylor-vann marked this pull request as ready for review June 2, 2022 20:28

taylor-vann requested a review from justinfagnani as a code owner June 2, 2022 20:28

taylor-vann requested a review from kevinpschaaf June 2, 2022 20:28

taylor-vann added 2 commits June 2, 2022 13:36

add changeset

0bdb617

more flexible record type for slots

d13aae3

taylor-vann self-assigned this Jun 2, 2022

kevinpschaaf requested a review from sorvell June 2, 2022 21:57

sorvell requested changes Jun 3, 2022

View reviewed changes

kevinpschaaf reviewed Jun 3, 2022

View reviewed changes

taylor-vann added 3 commits June 3, 2022 16:01

better naming

bd4834e

test for display: contents

3fd4390

pass createElement function instead of React module

05d4b5f

augustjk marked this pull request as draft January 12, 2024 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[@labs/react] support multiple children with multiple slots #2985

[@labs/react] support multiple children with multiple slots #2985

taylor-vann commented Jun 2, 2022 •

edited

changeset-bot bot commented Jun 2, 2022 •

edited

github-actions bot commented Jun 2, 2022 •

edited

📊 Tachometer Benchmark Results

Summary

Results

sorvell left a comment

sorvell Jun 3, 2022

sorvell Jun 3, 2022

taylor-vann Jun 3, 2022 •

edited

sorvell Jun 6, 2022

sorvell Jun 3, 2022

taylor-vann Jun 3, 2022

sorvell Jun 3, 2022

sorvell Jun 3, 2022

taylor-vann Jun 3, 2022

sorvell Jun 3, 2022

taylor-vann Jun 3, 2022

sorvell Jun 3, 2022

taylor-vann Jun 3, 2022

taylor-vann commented Jun 3, 2022

kevinpschaaf Jun 3, 2022

khanh-le-otsv commented Feb 17, 2023 •

edited

[@labs/react] support multiple children with multiple slots #2985

Are you sure you want to change the base?

[@labs/react] support multiple children with multiple slots #2985

Conversation

taylor-vann commented Jun 2, 2022 • edited

changeset-bot bot commented Jun 2, 2022 • edited

🦋 Changeset detected

github-actions bot commented Jun 2, 2022 • edited

📊 Tachometer Benchmark Results

Summary

Results

sorvell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylor-vann Jun 3, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taylor-vann commented Jun 3, 2022

Choose a reason for hiding this comment

khanh-le-otsv commented Feb 17, 2023 • edited

taylor-vann commented Jun 2, 2022 •

edited

changeset-bot bot commented Jun 2, 2022 •

edited

github-actions bot commented Jun 2, 2022 •

edited

taylor-vann Jun 3, 2022 •

edited

khanh-le-otsv commented Feb 17, 2023 •

edited