Skip to content

Row select and customized row select with prop feature.#39

Closed
erdembas wants to merge 1 commit into
Autodesk:masterfrom
nesbilgi:master
Closed

Row select and customized row select with prop feature.#39
erdembas wants to merge 1 commit into
Autodesk:masterfrom
nesbilgi:master

Conversation

@erdembas

Copy link
Copy Markdown

Our purpose of this pull request is to enable row select with pros.

Please review the PR.

We need your feedback to continue forward steps. Documentation, roadmap..

Our roadmap;
we want to make more flexable row select.

  • add onRowSelected event
  • add onRowDeSelected event
  • row click select
  • etc.

@nihgwu nihgwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PR looks promising, but I'd say I didn't include selection feature to BaseTable on purpose, as there are different behaviors for that, and I want to keep BaseTable unopinionated, for example:

  • should we show a indeterminate for the header's checkbox if partial is selected
  • as we support tree data, should we select all the children if the parent is selected, I'll tell you we don't in our advanced table
  • how to sync the staled keys if those rows are removed

We addressed those questions in our advanced table component based on BaseTable with our own behavior definition, see the screenshot here https://github.com/Autodesk/react-base-table#example

I'd prefer if we only add a example to demo the feature, I'll add a recipe for that, hope we could support it via plugin in the future

Comment thread src/BaseTable.js
super(props);

const { columns, children, expandedRowKeys, defaultExpandedRowKeys } = props;
const { columns, children, expandedRowKeys, defaultExpandedRowKeys, selectedRowKeys } = props;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be uncontrolled defaultSelectedRowKeys as well

Comment thread src/BaseTable.js
/**
* Selected row data key
*/
selectedRowKey: PropTypes.string,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unnecessary, should be the same as rowKey

Comment thread src/BaseTable.js
/**
* Selected row keys when initialize the table
*/
selectedRowKeys: PropTypes.arrayOf(PropTypes.oneOfType([PropTypes.string])),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the rowKey could be number now

@berkayakcay

Copy link
Copy Markdown

Hi, @nihgwu
I think that will be good if we support this feature we should guide the people.
Could you share best practise receipt for this feature.
Would you like to share it on example page?

@nihgwu

nihgwu commented May 27, 2019

Copy link
Copy Markdown
Contributor

@berkayakcay sure, I'll add a demo before we could support that via plugin

BTW, I'd like to share the APIs I'm using for selection feature
props:
image
method:
image

@erdembas

Copy link
Copy Markdown
Author

@nihgwu We want to clarify that you shared API's are available for your local brach. We couldn't see these API's on code base. Did you share your own API on your code base or you share these image to guide us while completing PR.
What should we do?

@nihgwu

nihgwu commented May 27, 2019 via email

Copy link
Copy Markdown
Contributor

@berkayakcay

Copy link
Copy Markdown

We decided to obey your suggestion. We create HOC and use a custom component. We will wait for "plugin support" #3
Thanks for your kind and fast responses. 🥇

@nihgwu

nihgwu commented Jul 2, 2019

Copy link
Copy Markdown
Contributor

hi @erdembas @berkayakcay, I've add a demo for selection feature, sorry for the long wait

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants