tleunen / react-mdl

React Components for Material Design Lite
https://tleunen.github.io/react-mdl/
MIT License
1.76k stars 255 forks source link

DataTable's `shadow` prop has unexpected 0 behavior #364

Closed milotoor closed 8 years ago

milotoor commented 8 years ago

The documentation has this to say about the DataTable's shadow prop: "Optional, Default 0. Must be between 0 and 6". However,

<DataTable rows={...}> ... </DataTable>

and

<DataTable shadow={0} rows={...}> ... </DataTable>

produce different results. Even though the shadow is clamped to 0 if it isn't supplied, the classNames utility won't add the shadow class to the class list because its addition is modulated by the hasShadow variable (which is false if no shadow prop was given).

This is pretty odd behavior to me. The default value, when specified, should produce the same result as when no value is specified at all-- pretty much the definition of default. I would rewrite the code that compiles the class list such that if the shadow prop is falsey no class is added, otherwise it is clamped from 1-7 and the correct shadowing is returned.

Happy to put a PR together.

tleunen commented 8 years ago

Hmm I believe the documentation is wrong. There's not really any "default" for this value because we need a way to not have any shadow at all.

Currently, the shadow system is quite bad, the value doesn't mean anything and I'm hoping to improve this in v2.

milotoor commented 8 years ago

Yes, agreed that the documentation is wrong. My proposed fix would produce the same behavior for

<DataTable rows={...}> ... </DataTable>

and

<DataTable shadow={0} rows={...}> ... </DataTable>

Namely, no shadow would be added to the table. The currently supported shadow values (those currently accessed by setting the prop from 0-6) would be remapped to 1-7. It's a breaking change, so I understand if you are reluctant to make the change prior to v2. Perhaps the best thing to do would just be to fix the docs?

milotoor commented 8 years ago

I can make a PR to amend the docs if you'd like.

tleunen commented 8 years ago

Sure, I'd like that. Thanks @milotoor.