Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

selectRow property improvement suggestions #278

Closed
dana2208 opened this issue Feb 16, 2016 · 2 comments
Closed

selectRow property improvement suggestions #278

dana2208 opened this issue Feb 16, 2016 · 2 comments

Comments

@dana2208
Copy link
Contributor

Hi!

I have 2 improvement suggestions concerning the selectRow property:

  1. From a semantic point of view, I think it would be more appropriate for selectRow.mode to receive "single" and "multiple" rather than "radio" and "checkbox".
    Indeed, it has a UI connotation (the way you want it to render) rather than a behavior connotation (the way you want it to behave). When specifying the mode I intend to tell the grid whether it supports single or multiple selection, which is the goal of the mode property. The fact that in the bottom line it renders a radio button or a checkbox is an implementation decision.
    It makes even more sense when you use hideSelectColumn: true, since you don't care about the rendering, but you do care that selection is single or multiple rows.
    Do you agree?
    By the way your own constant names are inline with the way I think, i.e. Const.ROW_SELECT_SINGLE is radio and Const.ROW_SELECT_MULTI is checkbox. As a user of your package I cannot use your constant values and therfore I have to write radio and checkbox.
  2. The _isSelectRowDefined function of TableBody.js checks whether the grid support selection by testing whether the mode property is one of the 2 values Const.ROW_SELECT_SINGLE or Const.ROW_SELECT_MULTI. In my point of view the right code should be:
    return (this.props.selectRow != undefined) which is enough for establishing whether grid supports selection or not.
    Maybe you had a good reason for checking for the mode in this location, if yes I would be glad you give me your reason.
    If the reason was to check that the mode provided was a legal value, then a better place to do it would be in BootstrapTable.propTypes or in TableBody.propTypes object definition.
    Instead of:
    selectRow: React.PropTypes.shape({
    mode: React.PropTypes.string,
    ...
    }),
    Write:
    selectRow: React.PropTypes.shape({
    mode: React.PropTypes.oneOf([Const.ROW_SELECT_SINGLE, Const.ROW_SELECT_MULTI]),
    ...
    }),

Thank you

@AllenFang
Copy link
Owner

@dana2208,
I agree this both suggestion, but I'll improve second suggestion first, because first suggestion will cause user to change their client code, so I prefer not to fix first suggestion currently, how do you think

btw, this component have a lot of improvement need to do, its all keep in my mind, like make es6 more compatible, testing, refactoring src/TableBody.js, performance tuning etc.

Feel free to discuss with me

@AllenFang
Copy link
Owner

@dana2208, btw for the second issue, I can't modify _isSelectRowDefined in src/TableBody.js for changing to return (this.props.selectRow != undefined); Because selectRow is always defined on BootstrapTable defaultProps.

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

No branches or pull requests

2 participants