Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize props, events, and slots #1621

Open
1 of 9 tasks
theetrain opened this issue Jan 10, 2023 · 11 comments
Open
1 of 9 tasks

Standardize props, events, and slots #1621

theetrain opened this issue Jan 10, 2023 · 11 comments

Comments

@theetrain
Copy link
Collaborator

theetrain commented Jan 10, 2023

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:

  1. ref or [elementName]Ref props
  2. Props that support 2-way binding ("reactive" variables)
  3. kind
  4. size
  5. Most important props, grouped with their complements such as labelText and hideLabel
  6. Least-used props
  7. "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 #1064 (comment)

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
@metonym
Copy link
Collaborator

metonym commented Jan 11, 2023

Linking this: #1420

@brunnerh
Copy link
Contributor

brunnerh commented Feb 4, 2023

Also related:

@metonym
Copy link
Collaborator

metonym commented Feb 9, 2023

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 theetrain mentioned this issue Jan 13, 2024
8 tasks
SimpleProgrammingAU pushed a commit to SimpleProgrammingAU/carbon-components-svelte that referenced this issue Jan 14, 2024
SimpleProgrammingAU pushed a commit to SimpleProgrammingAU/carbon-components-svelte that referenced this issue Jan 14, 2024
@theetrain theetrain mentioned this issue Jan 14, 2024
2 tasks
@theetrain theetrain changed the title Standardize props and events Standardize props, events, and slots Jan 14, 2024
@theetrain
Copy link
Collaborator Author

theetrain commented Jan 15, 2024

@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
Copy link
Collaborator

metonym commented Jan 15, 2024

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

SimpleProgrammingAU pushed a commit to SimpleProgrammingAU/carbon-components-svelte that referenced this issue Jan 17, 2024
theetrain added a commit that referenced this issue Jan 17, 2024
* Initial commit

* Fixes [FluidForm] TextInput error icon is misplaced #1667

* Contributes to [TextInput] helperText enhancements #1633

* Adopts Standardize props and events #1621

* Added slots for Standardize props and events #1621

* Added pointer events, updated skeleton TextInput v11 #1888

* Address a bug in the word counter regex

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Update src/TextInput/TextInput.svelte

Explicitly define default value for `size`

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Adopted suggested changes

* Updated `TextInput.test`; added forgotten files from previous

---------

Co-authored-by: Samuel Janda <hi@simpleprogramming.com.au>
Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>
@theetrain theetrain mentioned this issue Jan 19, 2024
4 tasks
SimpleProgrammingAU pushed a commit to SimpleProgrammingAU/carbon-components-svelte that referenced this issue Mar 8, 2024
@theetrain
Copy link
Collaborator Author

theetrain commented Mar 11, 2024

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.


Related comments: #1895 (comment), #1932 (comment)

cc @metonym @brunnerh @SimpleProgrammingAU

@SimpleProgrammingAU
Copy link
Contributor

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
Copy link
Contributor

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 #1895 (comment)). 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
Copy link
Collaborator Author

theetrain commented Mar 11, 2024

@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
Copy link
Collaborator

metonym commented Mar 11, 2024

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
Copy link
Contributor

brunnerh commented Mar 11, 2024

$$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.

metonym pushed a commit that referenced this issue Mar 23, 2024
* Initial commit

* Fixes [FluidForm] TextInput error icon is misplaced #1667

* Contributes to [TextInput] helperText enhancements #1633

* Adopts Standardize props and events #1621

* Added slots for Standardize props and events #1621

* Added pointer events, updated skeleton TextInput v11 #1888

* Address a bug in the word counter regex

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Update src/TextInput/TextInput.svelte

Correcting type attribute definition for HTML attributes

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Update src/TextInput/TextInput.svelte

Explicitly define default value for `size`

Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>

* Adopted suggested changes

* Updated `TextInput.test`; added forgotten files from previous

---------

Co-authored-by: Samuel Janda <hi@simpleprogramming.com.au>
Co-authored-by: Enrico Sacchetti <esacchetti@gmail.com>
@metonym metonym unpinned this issue Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants