Standardize props, events, and slots

theetrain commented 1 year ago

Status: 🟡 Proposal

We have several props and dispatched event names that are reused across components. Let's improve their consistency in naming and intent.

Todo

[ ] Combobox: change titleText to labelText

Prop ordering

Rather than alphabetical order, props SHOULD be ordered in a manner that is intuitive and useful to users. Sveld will generate documentation in the order props are defined within components.

Proposed ordering:

ref or [elementName]Ref props
Props that support 2-way binding ("reactive" variables)
kind
size
Most important props, grouped with their complements such as labelText and hideLabel
Least-used props
"Pass-through" props (see $$restProps below)

Prop names

Props that have different intent follow different aspirational rules.

The following MUST be followed:

Variants: use the name kind to represent a set of other named options for visual or interactive component representations
Use camelCase for multi-word prop names, but prefer single-word names
skeleton variants should be separate components; and not a prop toggle

The following SHOULD be followed:

Flags: props that toggle a feature via true or false values are represented as plain adjectives without an is prefix. Examples: expandable, selectable, selected. Prefer to have them default to false for easy truthy flagging. i.e. absense of selectable implies selectable == false, but the presence of selected toggles to true.
HTML attributes are provided via Type declarations and should not be included as explicit props unless it has special treatment as part of a component.
- Appropriate: disabled for <Button>
- Inappropriate: aria-label for <TextInput>
If a component contains an underlying interactive element, then a ref prop with 2-way binding should be provided to allow focusing via JS. Appropriate elements include, but aren't limited to:
- input
- button
- inline notification
Props with multiple options such as size should have all possible values explicitly typed, including defaults, such as 'sm' | 'md' | 'lg' instead of 'sm' | 'lg'. Components impacted:
- TreeView size
- ContentSwitcher size
- ExpandableTile...
- MultiSelect size
- OverflowMenu size
- Search size

Form components

Components that render form inputs MUST have the following props:

labelText for the label
helperText for any helpful or hint text placement
HTML attributes: name, id, value, checked
readonly should be readOnly

Prop behaviours SHOULD include:

value is bindable, and can be instantiated (currently not the case for Combobox)

Events

Demo: https://svelte.dev/repl/9b1e3a7822bb4aac9418c95f518523e7?version=3.55.1

In general:

prefer pointer events over mouse events to ensure cross-compatibility with touch and desktop devices. Use mouse or touch only when necessary

Dispatched events

Dispatched events MUST be named differently from native events

Forwarded events

Always forward useful native events. It would be nice if we had a way to forward all events easily (see RFC)
input-like components SHOULD forward on:input, on:submit, and on:change; this includes TextInput, ComboBox, and others

`$$restProps`

See https://github.com/carbon-design-system/carbon-components-svelte/issues/1064#issuecomment-1375954124

Avoid use of $$restProps since it's a discouraged feature of Svelte
Components SHOULD include a named prop for spreading element attributes, such as buttonAttributes for <Button>; and labelAttributes and inputAttributes for <TextInput>
See demo of "pass-through" props https://svelte.dev/repl/f6c1779a4b924686af69a4e9def81621?version=3.56.0

Slots

All props that accept copy SHOULD have named slots with the same name as their respective prop provided, to allow formatted text or icons to be added.
If a prop and named slot is provided, the slot has priority and should render instead

### Related tickets
- [ ] #1643
- [ ] #1611
- [ ] #950
- [ ] #314
- [ ] #925
- [ ] #1325
- [ ] #534
- [ ] Apply pass-through props to InlineNotification's NotificationActionButton
- [ ] #461

metonym commented 1 year ago

Linking this: https://github.com/carbon-design-system/carbon-components-svelte/discussions/1420

brunnerh commented 1 year ago

Also related:

1325
534

metonym commented 1 year ago

I agree that we should only forward useful events.

A lot of components have a wrapping div in which many events are forwarded to. We should not forward events in these cases as it generates a lot of boilerplate code. A user can always wrap the component with a div and listen to events as they bubble up.

Furthermore, it can be confusing if we forward events to different elements.

Example:

<!-- events forwarded to the div are not useful and can be done by the consumer -->
<div
    on:click
    on:mouseover
    on:mouseenter
    on:mouseleave
>
   <input on:input />
</div>

theetrain commented 10 months ago

@metonym I added "Prop ordering" above.

On another note, I noticed <Button> has a skeleton prop whereas other components like <TextInput> have a separate <TextInputSkeleton> component. Should we prefer props or components? I'd say components are better for tree-shaking. The user experience comparison would be:

As props:

<form method="POST" action="?/deleteProfile" use:enhance>
  <Button
    type="submit"
    kind="tertiary"
    size="lg"
    id="abc123"
    skeleton={!ready}
  />
    Delete Profile
  </Button>
</form>

As components:

{#if ready}
  <form method="POST" action="?/deleteProfile" use:enhance>
    <Button
      type="submit"
      kind="tertiary"
      size="lg"
      id="abc123"
    />
      Delete Profile
    </Button>
  </form>
{:else}
  <ButtonSkeleton size="lg" />
{/if}

metonym commented 10 months ago

@theetrain agree - skeleton variants should live as standalone components. We should remove the skeleton prop.

theetrain commented 8 months ago

Do we want to allow $$restProps for single-element components such as Button and Layer? Strictly speaking, it isn't a performance concern any more than attributes or buttonAttributes would be, and would allow for a less-cluttered API where possible.

Example with $$restProps:

<Button
  kind="secondary"
  type="submit"
  data-test-id="abc123"
>
  Save
</button>

Example without $$restProps:

<Button
  kind="secondary"
  type="submit"
  attributes={{ 'data-test-id': "abc123" }}
>
  Save
</button>

One other trade-off when using $$restProps is not having Carbon-specific props appear at the top of the autocomplete list; since it would alphabetize props and element attributes in one giant list.

cc @metonym @brunnerh @SimpleProgrammingAU

SimpleProgrammingAU commented 8 months ago

My personal preference is for a uniform API. I like the idea of being able to use CDS Svelte and know I should never use $$restProps without exception.

brunnerh commented 8 months ago

I like the idea of being able to use CDS Svelte and know I should never use $$restProps without exception.

It just shifts things, instead you now need to know which attribute object is the relevant one if any (because the delineation of special properties and plain attributes is not entirely clear - see https://github.com/carbon-design-system/carbon-components-svelte/pull/1895#discussion_r1458187996). These properties are meant to standardize, but this is just not possible to the degree envisioned.

The syntax is also just plain worse, it is more verbose, noisy and forces a switch away from HTML to JS in the markup, even for static attributes. This goes against Svelte's core HTML-first approach.

I would argue for the continued use of $$restProps where there is a logical candidate for receiving them. E.g. for any form elements the primary interactive element (e.g. TextInput -> <input>, Select -> <select>, Button -> <a>/<button>). Other elements that are important, like the root element for layout, a <label> or icon could have these additional attribute properties which hopefully should rarely need to be accessed at all.

In Svelte 5 rest props will further be strengthened with the ability to forward all event handlers. The feature was always meant to support the wrapping of elements in generic components, it does not make sense to me to ditch it.

theetrain commented 8 months ago

@brunnerh if I were to formalize your note into a protocol, it could be:

When components contain 1 interactive element, use $$restProps on that element, and add attribute props for other elements when requested or where it makes the most sense (such as label).
When components contain more than 1 interactive element (examples: DataTable, Pagination, UIShell), add $$restProps to the outer-most wrapping element, if any; and provide individual attribute props for all interactive child elements).

Mind you, some extra considerations may need to be made for #1622 (thinking about use: and adapting links, buttons, and form) and Svelte 5, but I think the above protocol sounds agreeable. Perhaps slots and snippets could reduce the need for attribute props even further.

Note: "attribute props" == "pass-through props"

metonym commented 8 months ago

Personally, I echo @SimpleProgrammingAU's sentiment.

$$restProps is discouraged because it's difficult to statically analyze/optimize.
Predictable API: is $$restProps forwarded to the "top-level" element or the "most important" element (e.g., an input or button)? Additionally, a prop named inputProps is very semantic.
Multiple spreading: what if I want to spread arbitrary props to multiple elements? Even if using $$restProps, this would inevitably lead to creating object props anyways, (e.g., menuProps etc..).

brunnerh commented 8 months ago

$$restProps is discouraged because it's difficult to statically analyze/optimize.

The same should be true for spreading any object as props. For component libraries there is no way around this.

Predictable API: is $$restProps forwarded to the "top-level" element or the "most important" element (e.g., an input or button)? Additionally, a prop named inputProps is very semantic.

It's not predictable because some props (e.g. disabled in Button as referenced) need special handling or are not simple attributes to be passed on to one element, so you might expect them on a *Props object but things will not behave as expected if you set them there.

Multiple spreading: what if I want to spread arbitrary props to multiple elements? Even if using $$restProps, this would inevitably lead to creating object props anyways, (e.g., menuProps etc..).

True, but ideally the secondary elements would not need as much customization.

carbon-design-system / carbon-components-svelte