The content property is not consistently implemented in all Stadust components

[mnajdova]

Bug Report

Steps

The content property is not consistent across all Stardust components. This can lead to confusion or incorrect usage of the Components when the content slot is in question.

Expected Result

I would expect that the content property should be used consistently in all Stardust components. As far as I remember, we decided to make the content property shorthand for the Slot component, and use it everywhere across our components. This would allow us to use the content as an actual shorthand slot inside each component.
On top of this, we are not even using consistent propTypes and interfaces for the content property. If we decide to go with the shorthand, then it can simple be defined as customPropTypes.itemShorthand in the propTypes and ShorthandValue in the props' interface.

Actual Result

Not all components support it. Actually only few components are following this convention. Let's take a look into two examples of a Component that supports it (Button) and of Component that do not support this (Divider):

Button component

Example code:

<Button content={{content: "Some text", as: 'span'}} />

Result:

Divider component

Example code:

<Divider content={{name: "Some text", as: "span"}} />

Result:
Error thrown

Version

0.12.0

Proposal

Let's try to make the content property to be shorthand for the Slot component in all Stardust components. If we see that this is not possible for some of the components, I would suggest we document it explicitly in the docs site, so that it won't create confusion to the users.

[bmdalex]

fixes #504 correct me if I'm wrong @mnajdova

[mnajdova]

Exactly, it should solve that issue as well.

[mnajdova]

Couple of additional thoughts. As this proposal would add additional dom element in the component, we must be sure that we are okay with it for all components. It seems kind a strange for the component that contains just the content in their structure. For example, let's take the Text component. Previously the rendered html was the following:

 <span class="ui-text">This text is light.</span>

After the changes, we have additional span rendered:

<span class="ui-text" dir="auto">
  <span class="ui-slot">This text is light.</span>
</span>

If we decide that we won't do this for some components however, we will stind end up with the previous problem. Any suggestions would be appreciated.

[bmdalex]

I think for some components (like Text, Label, Segment) it makes sense to have the content slot as a primitive type (mostly strings).

That's because it's pretty obvious (or should be) for the user that the content of a Text, Label (maybe less for Segment but it's not hard to grasp) should be just some text to be rendered in the main area.

[mnajdova]

Apart from the issue with the component containing only the content property, here is an overview for all other components regarding the content property:

Components not containing the content property:

• Accordion
• Attachment
• Avatar
• Chat
• Form
• FormField
• Icon
• Image
• Layout
• List
• Menu
• PortalInner
• Provider
• ProviderConsumer
• RadioGroup
• RadioGroupItem
• Ref
• Status

Components with custom content property

• Grid
• ItemLayout

Other problems found

• Label is using ItemLayout – content is not refactored to be slot, but it is used as main inside the ItemLayout’s main property.
• ListItem – same as Label

Question

• Can the MenuItem be refactor to use the content as a shorthand?
• Can the Portal be refactor to use the content as a shorthand?
• Label inside RadioGroupItem is used as a contentShrothand, again because of item layout is adding additional spans – should be refactored as the ChatMessage

[miroslavstastny]

MenuItem currently renders as following:

<wrapper = li>
  <ElementType = a>
    <Icon />
    content
  </ElementType>
</wrapper>

If we refactor content to be a shorthand, it will be wrapper by span, resulting in every item being li > a > span.
Is that what we want? Or do we want, in MenuItem.tsx, to check typeof content and leave it untouched if it is a string and create a Slot otherwise?

[mnajdova]

Is that what we want? Or do we want, in MenuItem.tsx, to check typeof content and leave it untouched if it is a string and create a Slot otherwise?

I would say the MenuItem is not the only component that needs this, as @Bugaa92 has mention there are other components that want the content to be just a string. But still, we should be very careful with this, because if the content is string, then we are not providing any way for it to be targeted and styled by the user, moreover we are not allowing the async rendering of the content (currently provided by the renderContent method, or the refactored approach, by using the shorthand as a function).If we are okay with this, we should document this clearly and make it consistent throughout all components.

Other components containing only the content property in the root

• Text
• PopupContent
• Header – can contain only the content, by conditionally can add the description as well (should we add it as a shorthand or just to use the content as a string?) we won’t be able to target only the header content (without the description if we don’t add it).
• HeaderDescription
• PopupContent
• Divider
• AccordionContent – removing the slot shorthand from here, makes it inconsistent with the AccordionTitle :\

[bmdalex]

For the Layout related issues:
I feel we should remove Layout and ItemLayout from our codebase altogether :)
Refactoring Chat.Message to not use them was not that difficult and we should think if these components provide more benefits than cons; related to #241 and #477

[levithomason]

Yes, this is a tough issue. Here is more conversation and design on the same topic:

Standardize content prop: Semantic-Org/Semantic-UI-React#1364

Standardize shorthand prop names for primary content: Semantic-Org/Semantic-UI-React#391

The conclusion in those issues was to leave the inconsistency we have in favor of more correct markup. After our offline discussions here at Stardust, we still think the same thing.

[levithomason]

Here's another summary and comparison of the content prop:

REMOVE? content is only aliasing children
NECESSARY? content is node composed into children
NECESSARY? content is a component (rename to body?)

Shorthand usages:
Menu Item === { icon: '', content: '' }
Button === { icon: '', content: '' }
Divider === { children: '' }
Slot === { children: '' }
Popup === { body: {} }
Message === { body: {} }

This API would be more confusing to users. Also, if the component changes its markup, it has to break its API typings. Let's stick with content for now and only change typings 👍

[levithomason]

@miroslavstastny I believe you had a closing comment on this topic after we discussed the above (see my last comment). Can you post here and close this issue?

[levithomason]

The conclusion in [Semantic UI React's] issues was to leave the inconsistency we have in favor of more correct markup. After our offline discussions here at Stardust, we still think the same thing.