Eui: EuiTable[X]Cell interfaces lack needed attributes

Created on 15 Oct 2019  路  8Comments  路  Source: elastic/eui

EuiTableHeaderCell, EuiTableFooterCell, EuiTableRowCell, and possibly others are missing types for properties such as width and height.
The typical, in-use method of extending TdHTMLAttributes<HTMLTableCellElement> is insufficient as the majority of HTMLTableCellElement properties don't get extended.
If we were to extend HTMLTableCellElement directly, children becomes conflicted, and several other optional properties become required. Partial and Omit may cover our needs.

It's possible that other table sub-element DOM interfaces are similarly incorrect/insufficient.

bug typescript

All 8 comments

Of interest: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/td
Pretty much all of the attributes in question are deprecated or obsolete.

For width (the main attribute in question), the recommendation is use CSS instead. We may be better served to accept width, but implement it via style

@chandlerprall What're your thoughts on EUI's intended public API for these types components? And would it constitute a breaking change if we were to consciously (it's already happened unintentionally with the conversion to TS) remove support for attributes obsolete as of HTML5 (width, for instance)?

  • We spread props onto HTML elements, which gives the assumption that any valid attributes are supported. Standards are in flux, though.
  • We've adhered to modern standards on attributes like align (obsolete), transforming it from a DOM attribute to a CSS rule. We could do this across the remaining attributes.
  • We've explicitly added the colSpan attribute (not obsolete or deprecated) to the component API, even though it would've been supported as-is via ...rest.

Why not just implement as style but allow consumers to set width? The whole point of EUI is to abstract the implementation details like this in favor of a simpler API. As a consumer, I don't care

Yes, to clarify, I think width needs to be supported and done so via style. It's the other, more obscure attributes that we've _seemingly_ supported that I'm questioning.

I think we should lean toward respecting the fact that these attributes are deprecated. If a user really wants to pass them they can with // @ts-ignore and they'll be forwarded as normal. Likewise, the style prop should already be forwarded and allow width to be set that way.

Not supporting an explicit width prop on the component alleviates the headaches with merging it into a potentially-already-existing style prop and continues to endorse the HTML spec's recommendations (and deprecations).

That said, I expect my answer causes some frustration and/or additional work for a person or people, so I'm open to push back :)

Setting column width is a super basic aspect of creating custom tables. As a consumer, I don't care that EuiTableHeaderCell becomes an actual <th style='width:20%'> when I do <EuiTableHeaderCell width='20%'>. Having to know this implementation detail is problematic for a high level interface like EUI because I expect the interface to abstract me from these details, so that the underlying implementation can change without me knowing or caring. For all I care, the table parts could all be <div> elements.

Passing style makes me uneasy because it signals that the implementation could change at any moment and my custom style might stop working, might start being applied on a different element that takes {...rest} or the DOM level props, etc, etc, etc. Passing something like width, as a high level consumer I'm saying I want these columns to be so-and-so in width, I don't care how you do it. The fact that there's a deprecated DOM level width attribute is anecdotal.

Like, forget the DOM, consumers need a simple way of specifying how much space each column takes relative to the others, and width is a simple enough abstraction for that. Even if this were VB6 forms or Android views, you'd want that

@bevacqua makes good points, as always. Looking deeper, EUI did actually add explicit support for width to EuiTableHeaderCellCheckbox, so at the very least we're inconsistent. I'd rather add it to EuiTableHeaderCell and EuiTableFooterCell than remove it from where it exists.
I'll open an initial PR that resolves width via style, but does not add support for other obsolete td/th attributes.

Fair enough, and I definitely agree with your points in the first paragraph. I'd argue that EUI already leaks (by design) a lot of the underlying details through TypeScript & the ...rest forwarding we do, but to your point that doesn't mean we have to rely on it in any applicable case (nor do we in many other places).

Supporting width as a prop on these components makes sense, as @thompsongl just beat me to :)

Was this page helpful?
0 / 5 - 0 ratings