Skip to content

Conversation

AllenFang
Copy link
Member

@AllenFang AllenFang commented Oct 23, 2017

@Chun-MingChen

Sorry that I make a mistake about React.cloneElement.. I expect to use React.cloneElement to wrap the element again and again for the reason of separating the heavy logic to different and small components.

So I defined the three wrapper:
./packages/react-bootstrap-table2/src/sort/wrapper.js
./packages/react-bootstrap-table2/src/row-selection/wrapper.js
./packages/react-bootstrap-table2/src/cell-edit/wrapper.js

And in the render method of ./packages/react-bootstrap-table2/src/container.js, I use React.cloneElement to wrap a react element dynamically(I mean it depend on props that user given)

Actually all of those stuff is work until I found a bug when we render with one more wrappers,
for example, if user define a table with sort and row selection or
a table with row selection and cell edit, the bug will be produced...
However, if a table only render with one wrapper, that will be fine. That's why some lately to found this bug..

Let me know your idea or if there's any unclear and I'll fix the testing in these couple days(tomorrow)

@AllenFang AllenFang added the bug label Oct 23, 2017
@AllenFang AllenFang added this to the 0.1.0 milestone Oct 23, 2017
@AllenFang AllenFang requested a review from chunming-c October 23, 2017 16:26
@AllenFang AllenFang changed the title Fix wrong higher order wrapping [WIP] Fix wrong higher order wrapping Oct 23, 2017
@chunming-c
Copy link
Member

chunming-c commented Oct 23, 2017

@AllenFang, haha I just found that and I'm ready to creating an issue. I thought wrapper was a great implementation and it allow bootstrap-table to be more concise and easier to understand. Let me study your PR first :)

@AllenFang AllenFang force-pushed the bugs/fix-wrong-higher-order-wrapping branch from 82b3a59 to 30fba35 Compare October 24, 2017 05:51
@AllenFang AllenFang changed the title [WIP] Fix wrong higher order wrapping Fix wrong higher order wrapping Oct 24, 2017
@AllenFang
Copy link
Member Author

This solution is work but we can do it more better.
Because for example: if a user defined a table only with cellEdit(No selection and no any sort column), The current patch will wrap CellEditWrapper, RowSelectionWrapper and SortWrapper, but last two wrappers are not necessary, due to user do not defined selectRow and sort,

In the future, I think we can only wrap CellEditWrapper but need some code to handle it.

@AllenFang AllenFang merged commit bbeb122 into develop Oct 24, 2017
@AllenFang AllenFang deleted the bugs/fix-wrong-higher-order-wrapping branch October 24, 2017 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2 participants