glittershark / reactable

Fast, flexible, and simple data tables in React
glittershark.github.io/reactable
MIT License
1.51k stars 222 forks source link

Warning: Unknown prop `props` on <td> tag. #311

Closed hassankhan closed 7 years ago

hassankhan commented 8 years ago

Seems like perhaps a few elements got missed?

Daniel-Baranowski-BJSS commented 8 years ago

I got the same problem. Warning: Unknown proppropson <td> tag. Remove this prop from the element. For details, see https://fb.me/react-unknown-prop in td (created by Td) in Td (created by Tr) in tr (created by Tr) in Tr (created by Table) in tbody (created by Table) in table (created by Table) in Table (created by HorseRacingRunnersTable) in HorseRacingRunnersTable (created by HorseRacingRacecard) in section (created by HorseRacingRacecard) in div (created by HorseRacingRacecard) in HorseRacingRacecard (created by Connect(HorseRacingRacecard)) in Connect(HorseRacingRacecard) (created by Connect(Connect(HorseRacingRacecard))) in Connect(Connect(HorseRacingRacecard)) (created by RouterContext) in section (created by App) in div (created by App) in App (created by RouterContext) in RouterContext (created by AsyncConnect) in AsyncConnect (created by Connect(AsyncConnect)) in Connect(AsyncConnect) in Provider

hassankhan commented 8 years ago

It seems this has been done in a bit of a borked way ... people have updated certain elements at a time. #306 actually seemed like it did a thorough job, but yeah, perhaps time for another PR?

vdh commented 8 years ago

@hassankhan #306 only "fixed" the issue because it completely removed the props spread on the <td> element.

The issue seems to be caused by thead.jsx creating a value called props alongside key and label in the column values, which then gets incorrectly merged into the props inside td.jsx.

I would attempt to fix this but I'm confused about the logic behind this column prop sharing code to begin with.

vdh commented 8 years ago

Okay, so…

This is the (venerable) commit that created the inheritance from the column prop. But then this commit added column.props to thead.jsx without updating the logic in td.jsx, thus creating this issue.

hassankhan commented 8 years ago

Yeah it seemed like something weird had happened, thanks for investigating 👍

fijolekProjects commented 8 years ago

I've got:

Warning: Unknown props `previousPageLabel`, `nextPageLabel` on <table> tag. Remove these props from the element. For details, see https://fb.me/react-unknown-prop

Is this the same issue as above or maybe I should create another issue?

Daniel-Baranowski-BJSS commented 8 years ago

@glittershark Are you planning on fixing this issue? Its been 20 days and we didn't get any response from you.

vdh commented 8 years ago

@fijolekProjects I have a fix for that one other issue pending in PR #314

vdh commented 8 years ago

@glittershark You should probably re-open this issue. #314 didn't have a fix for this issue, only the unrelated issues mentioned by others.

I haven't attempted to fix this props on td issue because I don't know what the original logic behind the code was. I didn't want to break things further trying to bodge something I didn't understand. I've managed to trace the issue back to their original commits and the commit that broke it, but the design purpose of it all is still uncertain to me. I was hoping you would know how to fix this one better than I could.

glittershark commented 8 years ago

I'm gonna take a closer look at this once I clear some time (potentially later this week). As I'm sure you've noticed, work has been taking up a lot of my time in the last few months :smile:

kinyat commented 8 years ago

The following code will generate the same error

            <Table
                className="hover table-full-width"
                data={[
                    { Name: 'Griffin Smith', Age: 18 },
                    { Age: 23,  Name: 'Lee Salminen' },
                    { Age: 28, Position: 'Developer' },
                ]}
            >
                <Thead>
                    <Th column="Name"/>
                    <Th column="Age" className="fit"/>
                    <Th column="Position" className="fit"/>
                </Thead>
            </Table>
kinyat commented 8 years ago

Will it be a problem to just ignore the key props when merging props in td?

In reactable/lib/reactable/td.js, line 55, add a check for props

if (key !== 'key' && key !== 'name' && key !== 'props') {
    mergedProps[key] = this.props.column[key];
}

That will get rid of the warning but not sure if this will cause any issue

hassankhan commented 8 years ago

IIRC, the recommended way is to have a list of properties for the component, and to remove those properties from the wrapped component. Could be wrong, I think I saw it done that way in react-bootstrap.

lgraubner commented 8 years ago

Any fix coming soon?

hassankhan commented 8 years ago

I believe it has been fixed, but there hasn't been a new release on npm yet.

lgraubner commented 8 years ago

Cool, hope to see it on npm soon. 👍

vdh commented 8 years ago

@hassankhan @lgraubner No fix yet, someone will have to tackle the logic conflict between those two commits I highlighted earlier. It looks like @glittershark has a busy schedule so if anyone wants it fixed sooner it would be better to help out with a PR.

rizkiandrianto commented 8 years ago

Any update guys?

ericf89 commented 8 years ago

I think I fixed it with my PR, but I'm not sure if I broke any other intended behavior. You can try it locally with "reactable":"git://github.com/ericf89/reactable.git#aab6bcbe05bdf5ac02d5673dbac51cb008bac01a" in your package.json.

ghost commented 8 years ago

Hello, @vdh or @glittershark could you take a look at PR #328, I used @ericf89's version on a table with customized almost everything (thead, tbody, tfoot, pagination, filtering and sorting), and it doesn't seem have any regression to me.

highwaycoder commented 7 years ago

@ericf89 works for us, looking forward to this making it into a release :)

glittershark commented 7 years ago

Just pushed 0.14.1, can you all confirm that fixes this issue?

hassankhan commented 7 years ago

I can confirm, no more warnings 👍

glittershark commented 7 years ago

woo! 🎉

highwaycoder commented 7 years ago

Confirmed working with 0.14.1, updated package.json this morning :) :tada: