Add the possibility to style Table rows by providing a 'style' attribute

[codecov-io]

Current coverage is 99.36% (diff: 100%)

Merging #398 into master will increase coverage by 0.01%

Diff Coverage	File Path
•••••••••• 100%	src/DataTable/tests/Table-test.js
•••••••••• 100%	src/DataTable/Table.js

Powered by Codecov. Last update 1be1492...87b0772

[Augustin82]

@tleunen any update on how I should proceed? Do I need to write more tests, to fix the handling of edge cases, to argue my case some more?

Please let me know and I'll get to work. Cheers!

[tleunen]

Sorry @Augustin82, I've been busy with some other things recently and couldn't spend much time on react-mdl :s

So after a quick investigation (to remind me how the Table works), className is mostly used internally to set the row as "selected". While I believe it's ok, I also think it could conflict with the row data, especially if the row data already has a className or style property. I believe for v1, we should be fine, but for v2, I'd prefer having a special prop, maybe something like __mdlRowProp or something very unlikely to be used as a data prop to avoid any conflict.

Thoughts?

[Augustin82]

@tleunen no worries, I was just pinging you. Thanks for taking the time to answer.

I agree with what you said on the need for v2 to have a special prop that would help limiting conflicts (along with explicitly defining it in the documentation).

Would it be OK, for the current version, to release support for "proper" React styling, still? I could update my PR to use a less obvious name than style (such as __mdlRowClassName and __mdlRowStyle, or something like you suggested).

The same thing could be extended to individual cells if deemed necessary; again, I could update my PR to support this.

Thanks!

[tleunen]

On a cell basis, it could be a bit more complicated to do so. To allow that, each cell data must be an object, with the special style key inside. Which will require users to create a new array of objects. Initially, the way I designed the table was especially to be able to take the raw data from an API/DB and use it inside the table with only the keys we want to display, without manipulating the data (or manipulating while we're displaying with the cellFormatter).

It really depends on the use case, if there's one important use case for that, we might support it, otherwise it doesn't look necessary. Most of the time, a user should be able to achieve what he wants with a custom cellFormatter.

I believe your implementation is fine for now, maybe just check style is an object before adding it in the element? And maybe rename it with _mdlRowProps, while adding support for custom className and style? Having a generic props would also allow to add onClick event and other things on the tr

[Augustin82]

OK, I'm on it. Thanks for the feedback and suggestions =)

[Augustin82]

Updated. What do you think, @tleunen ?

[Augustin82]

Updated as per your comments. Thoughts?

[Augustin82]

@tleunen sorry to ping you, any update on this?

[tleunen]

Sorry I was away this weekend and came back yesterday, will make sure we merge this asap ;)

[Augustin82]

Is there any problem with it?

[Augustin82]

@tleunen any update on this, please?

[tleunen]

I thought I asked why doing {...{ style, onClick }} instead of just passing all props { ...mdlRowProps } because if the user wants another event on it, like onDoubleClick or onMouseOver, we'll also need to add them. We can't really maintain a whitelist imo.

[Augustin82]

Apologies, I didn't understand it that way.

I have pushed an update, is that what you mean?

Thanks!

[tleunen]

Thanks @Augustin82 and sorry again for the long long delay

[Augustin82]

No need to apologize. Thank you very much for taking the time to review it, suggest correction, review it again because I messed up, and merging it ;)