Item Collection Wrapped Children

[ajlende]

Bug Report

Description

I wanted to create a custom item component for the item collection, but wrapping the <ItemCollection.Item /> component results in incorrect HTML being generated (<li /> instead of <tr />).

The workaround of just inlining the component is straightforward, but I didn't expect wrapping the item in a component to break the code.

The example below is stripped down, but with a more complex item, it would be nice to break the item out into its own component for readability, if possible.

Steps to Reproduce

Short, Self Contained, Correct (Compilable), Example (produces incorrect results)

import React from 'react';
import ItemCollection from 'terra-clinical-item-collection';

const MyCustomItem = () => (
  <ItemCollection.Item>
    <ItemCollection.Display text="Custom Display" />
    <ItemCollection.Display text="Another Display" />
  </ItemCollection.Item>
);

const App = () => (
  <ItemCollection numberOfDisplays={2}>
    <MyCustomItem />
  </ItemCollection>
);

export default App;

Workaround (produces correct results)

import React from 'react';
import ItemCollection from 'terra-clinical-item-collection';

const App = () => (
  <ItemCollection numberOfDisplays={2}>
    <ItemCollection.Item>
      <ItemCollection.Display text="Custom Display" />
      <ItemCollection.Display text="Another Display" />
    </ItemCollection.Item>
  </ItemCollection>
);

export default App;

Additional Context / Screenshots

Error Message

Warning: validateDOMNesting(...): <li> cannot appear as a child of <tbody>.
    in li (created by ListItem)
    in ListItem (created by Item)
    in Item (at App.js:16)
    in Message (at App.js:28)
    in tbody (created by TableRows)
    in TableRows (created by TableView)
    in table (created by Table)
    in Table (created by TableView)
    in TableView (created by ItemCollection)
    in ResponsiveElement (created by ItemCollection)
    in div (created by ItemCollection)
    in ItemCollection (at App.js:27)
    in div (created by Scroll)
    in Scroll (created by ContentContainer)
    in div (created by ContentContainer)
    in div (created by ContentContainer)
    in ContentContainer (at App.js:26)
    in div (created by I18nProvider)
    in IntlProvider (created by I18nProvider)
    in I18nProvider (created by Base)
    in Base (at App.js:24)
    in App (at src/index.js:6)

Generated HTML

<div id="root">
    <div>
        <table data-terra-clinical-item-collection-table-view="true" class="Table-module__table___2z3rN ItemCollection__table___3S1na">
            <colgroup>
                <col data-terra-clinical-item-collection-display-column="true">
                <col data-terra-clinical-item-collection-display-column="true">
            </colgroup>
            <tbody>
                <li class="List-module__list-item___1Gt4b">
                    <div class="ItemView__item-view___3_w3c ItemView__one-column___25Mjp">
                        <div class="ItemView__body___25Fo2">
                            <div class="ItemView__row-container___3P3oM">
                                <div class="ItemView__row___13eNI">
                                    <div class="ItemView__content___1akvI ItemView__content-primary-size___SO5lm ItemView__content-primary-color___1huX6">
                                        <div class="ItemDisplay__item-display___147d4">
                                            <div class="ItemDisplay__text___31JJn">Custom Display</div>
                                        </div>
                                    </div>
                                </div>
                                <div class="ItemView__row___13eNI">
                                    <div class="ItemView__content___1akvI ItemView__content-secondary-size___J4H8I ItemView__content-secondary-color___3T3fJ">
                                        <div class="ItemDisplay__item-display___147d4">
                                            <div class="ItemDisplay__text___31JJn">Another Display</div>
                                        </div>
                                    </div>
                                </div>
                            </div>
                        </div>
                    </div>
                </li>
            </tbody>
        </table>
    </div>
</div>

Expected Behavior

Expected HTML

<div id="root">
    <div>
        <table data-terra-clinical-item-collection-table-view="true" class="Table-module__table___2z3rN ItemCollection__table___3S1na">
            <colgroup>
                <col data-terra-clinical-item-collection-display-column="true">
                <col data-terra-clinical-item-collection-display-column="true">
            </colgroup>
            <tbody>
                <tr aria-selected="false" class="Table-module__row___1Ey4O">
                    <td class="ItemCollection__content-display___2Uzbm" data-terra-table-cell="true">
                        <div class="ItemDisplay__item-display___147d4">
                            <div class="ItemDisplay__text___31JJn">Custom Display</div>
                        </div>
                    </td>
                    <td class="ItemCollection__content-display___2Uzbm" data-terra-table-cell="true">
                        <div class="ItemDisplay__item-display___147d4">
                            <div class="ItemDisplay__text___31JJn">Another Display</div>
                        </div>
                    </td>
                </tr>
            </tbody>
        </table>
    </div>
</div>

Possible Solution

Environment

• Component Name and Version: terra-clinical-item-collection@3.16.0
• Browser Name and Version: N/A
• Node/npm Version: N/A
• Webpack Version: N/A
• Operating System and version (desktop or mobile): N/A

[emilyrohrbough]

Have you tried using the terra-responsive-element to toggle the list vs table item markdown? That is what currently managing which layout should be rendered.

[ajlende]

@emilyrohrbough I came across this when copying code from Kaiju and turning the various pieces into components. The problem for me isn't so much that I can't create a component, but it's more that I would expect copying code in-verbatim into a component to "just work". Instead I get this error and couldn't find any documentation explaining why I can't wrap the component.

[StephenEsser]

Hi @ajlende

My assumption is you need to allow properties to pass through your component wrapper onto the Item. The ItemCollection clones each Item and copies special props into them. Try opening up your custom component to accept properties.

const MyCustomItem = (props) => (
  <ItemCollection.Item {...props}>
    <ItemCollection.Display text="Custom Display" />
    <ItemCollection.Display text="Another Display" />
  </ItemCollection.Item>
);

[StephenEsser]

There are a few additional thoughts I wanted to add here to address why copying code into a custom component does not always work.

It's important to note that creating custom component wrappers can break API contracts.

As demonstrated in your example, removal of the property spread resulted in a malformed component structure and functionality.

The other common contract broken with custom components is type checking. If the ItemCollection needed to ask the child its type it would return MyCustomItem and not ItemCollection.Item

// This logic statement would fail if a component wrapper was used
if(child.type === 'ItemCollection.Item`) {
 // ...
}

For this specific case of the item collection I don't see any immediate issues resulting from creating a custom component. But it should be done with caution and with a thorough understanding of possible side effects.

It is very possible that in the future the ItemCollection may need to perform type checking on its children.

[ajlende]

@StephenEsser What you said makes sense. This is more of a documentation issue than anything else, I'd say. Exactly what ItemCollection and ItemCollection.Item are doing (and what they are expecting) isn't clear from just reading the documentation and examples.

[emilyrohrbough]

@ajlende would you want to open a PR to beef up the documentation so they are a bit clearer?

[ajlende]

@emilyrohrbough I can do that when I have some time. Might be a bit before I can get to it.

[StephenEsser]

@ajlende

This is somewhat of a challenging dilemma regarding documentation. In my opinion this is not a direct issue with the ItemCollection documentation but a generic expectation of all components that have subcomponents.

There is an implied expectation that certain components are expecting certain children. Passing in unexpected content challenges the API contract.

A List component is expecting List Items A Table component expects Rows, Columns, and Cells A Select component expects Options

These expectations also apply to native html

<select>
  <option />
</select>

<ul>
  <li />
</ul>

There are dozens of more examples throughout the Terra ecosystem.

I don't know whether or not explicitly documenting that each of these components is expecting specific children is sustainable or appropriate to display in the terra site pages, but I do think there is some value in documenting at a high level the general overview of sub components.

Let me know what your thoughts are and any possible solutions you may have that might help solve this confusion for other components as well.

[tbiethman]

If you're just trying to encapsulate some of your component creation, you could forgo creating a separate wrapping component and simply use functions to avoid those problems:

import React from 'react';
import ItemCollection from 'terra-clinical-item-collection';

const buildCustomItem = () => (
  <ItemCollection.Item>
    <ItemCollection.Display text="Custom Display" />
    <ItemCollection.Display text="Another Display" />
  </ItemCollection.Item>
);

const App = () => (
  <ItemCollection numberOfDisplays={2}>
    {buildCustomItem()}
  </ItemCollection>
);

export default App;

This simplicity breaks down quick if you need hooks into the lifecycles and whatnot, but I agree with Stephen in that I don't think more documentation will solve the problem completely. While we do our best to maintain consistency with our consumer-facing prop APIs, sometimes that forces us into a variety of internal implementation strategies that will have myriad impacts on things like wrapping/cloning/general manipulation of components passed in as props.

I do think that that updating the List/Table/etc. docs to say "The List will only support List.Item components as children" would be beneficial and might have saved you some time. I just think anything else would be hard to maintain and more than likely confusing to all but the most experienced consumers.

[ajlende]

@StephenEsser Thanks for pointing out that this probably isn't limited to just ItemCollection. If there are other Components that require specific children, then it might be worth adding a section to all Components with a requirement like that. MDN documentation has a section called "Permitted Content" on HTML Elements to show which Elements are allowed within an HTML Element (for example, ul)—doing something similar in the Terra docs seems like a reasonable solution to me.

Additionally, if the Terra team wants to be more strict about which children are allowed, you could always add a custom prop-types check and throw an error when there is an unacceptable child. 🤷‍♂️

And @tbiethman, your example is basically what I landed on—just inside an Array.map 😄

[tbiethman]