Sanity: [components] Inconsistent naming convention for boolean props

[bjoerge]

Right now some of our components expects props like scrollable, while others expect e.g. isOpen. We should agree on one convention and stick to it.

Personally, I prefer isScrollable over scrollable.

Opinions?

---

This issue has been concluded with: https://github.com/sanity-io/sanity/issues/20#issuecomment-337169189

If the semantics is 1:1 with native, use the native prop name, otherwise, use is/has/*-prefix

[rexxars]

+1 for is-prefix

[bjoerge]

So, one potential source of confusion is components that represents a lower level component, like e.g. our TextInput component, which to some extent can be thought of as a regular <input type="text"> with a few added features. Since HTML inputs takes a disabled-attribute, it may be a bit confusing that <TextInput type="email" disabled> won't work, and that you instead need to write <TextInput type="email" isDisabled />.

I still think I'm in favor of is/has, though. We could also support both regimes, or show a warning.

Thoughts?

[thomax]

Still +1 for is-prefix. The HTML spec is wrong :)

[kristofferj]

We should support native HTML-props if the the context and behaviour is similar to well known HTML conventions (disabled, checked etc). All other bool-props that we make should use is/has-prefix. Some components are just wrapped HTML-elements, and can be expected to inherit all the props.

[bjoerge]

Re: supporting both regimes: That would probably just create more headache in the long run.
Consider <TextInput isDisabled={false} disabled={true} ... />. What to choose? If we go for is-prefixes only, I'm all for warning in __DEV__ on receiving disabled and other boolean attributes.

[bjoerge]

@kristofferj I see your point. Still, I think I'd feel a little bummed out if isDisabled didn't work here :/

<TextInput
  ...
  hasFocus
  isDisabled
  isSelected
/>

The following doesn't quite feel right either, and the inconsistency would certainly look like a bug to me:

<TextInput
  ...
  hasFocus
  disabled
  isSelected
/>

[kristofferj]

I think we should support both.

[bjoerge]

I think we should support both.

What should happen here? <TextInput isDisabled={false} disabled={true} ... />

I realized we will never achieve complete consistency in any case (e.g. event.target.checked on onChange-handlers will still not be prefixed), so the goal should be to reduce ambiguity and avoid inconsistencies where we can.

My vote is still for consistent is/has/can-prefixes, even for props that have native counterparts. Keep in mind that React has already diverged from native naming conventions in HTML by only supporting camelCased props, even those who represents native events (e.g. onChange, tabIndex), and we're all gotten used to it by now.

[bjoerge]

The more I think about this, the more i agree with @kristofferj's argument here:

We should support native HTML-props if the the context and behaviour is similar to well known HTML conventions (disabled, checked etc). All other bool-props that we make should use is/has-prefix. Some components are just wrapped HTML-elements, and can be expected to inherit all the props.

As a bonus it becomes really easy to spot what props are intended to be passed down to underlying native form controls and what props are originating from the sanity codebase.

[bjoerge]

tl;dr: If the semantics is 1:1 with native, use the native prop name, otherwise, use is/has/*-prefix