[V4] EditableCell improvements

[jeroenransijn]

Implements: #275.

Table.EditableCell

• Add disabled property.
• Add placeholder property.
• Make isSelectable optional.

Table.Cell

• Add rightView property, mainly for icons + icon buttons.

Table.SelectMenuCell

• Add default caret-down icon for the rightView property.

[mshwery]

Out of curiosity, what's this impetus to add this to v4?

[mshwery]

You should check that this has been passed in and is a function first, since it's not marked as required nor does it have a default prop value:

if (typeof this.props.onEditComplete === 'function') {
  this.props.onEditComplete()
}

[mshwery]

This seems excessive to me – why not use an actual input element or a contenteditable div? This would all be built into the component and you'd get normal onChange handlers.

[mshwery]

I see you are you a textarea, but binding to onKeyDown. Can you explain your decision there a bit more?

[jeroenransijn]

This can be a bit confusing! EditableCell is actually just a selectable Table.TextCell. As soon as you hit enter, space or a character — a component called EditableCellField which contains a Textarea is overlayed exactly on top of the table cell. The EditableCellField position will be kept in sync. The reason for matching the key and passing the key as the value is then passed to the Textarea. This is inline with how spreadsheets work.

[mshwery]

You could probably do return typeof this.props.isEditing === 'boolean' if that's the expected type, and even doing 'isEditing' in this.props might be clearer semantics for what you are looking for here.

[mshwery]

Anytime you allow a component to be fully controlled OR fully uncontrolled you need to make some careful decisions around what happens when consumers switch a mounted component from one to the other. This could be as simple as a consumer unintentionally passing an undefined value to one of the controlled props, so the component would start out uncontrolled, and as soon their actual value comes in the component would be switched to controlled.

How will this component's value be affected when you switch from controlled to uncontrolled or vice versa?

[mshwery]

Beware of some of these pitfalls (which are commonly used when you have controlled and uncontrolled support side-by-side): https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-unconditionally-copying-props-to-state

[mshwery]

One implementation I've seen that helps tease apart concerns is to actually make a *Stateless component that is fully controlled, and a separate component for holding any internal state (uncontrolled, in this case).

[jeroenransijn]

The reason for this hitting v4 is some use cases within our app (tracking plan) that require editable cells. I wanted to explore the icon button as a trigger instead of double click to increase affordability in some cases. However, the implementation of controlled/uncontrolled feels janky and prone to errors. I will remove that usage for now.

Future idea

Instead of making the component controlled, we could opt-in for something like the following:

<Table.EditableCell
  isSelectable={false}
  rightView={({ startEditing }) => <IconButton onClick={startEditing} ... />}
  onChange={this.handleChange}
  ...
/>

[montogeek]

Where can I read the docs for this feature?

[mshwery]

@montogeek good call out! I don't think we have any documentation of this feature yet?