Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

issue175 editable column#182

Merged
Marc-Andre-Rivet merged 8 commits into
masterfrom
3.1-issue175-editable-column
Oct 30, 2018
Merged

issue175 editable column#182
Marc-Andre-Rivet merged 8 commits into
masterfrom
3.1-issue175-editable-column

Conversation

@Marc-Andre-Rivet

@Marc-Andre-Rivet Marc-Andre-Rivet commented Oct 29, 2018

Copy link
Copy Markdown
Contributor

This fixes #175

  • Change isEditable logic so that a cell is editable/not editable when the cell 'isEditable' property is true/false, and use table 'isEditable' prop if undefined.

  • additional units tests for the editable logic

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 29, 2018 20:43 Inactive
editable: boolean,
column: IVisibleColumn
): boolean => editable && (column.editable === undefined || column.editable); No newline at end of file
editableColumn: boolean | undefined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removing the dependency on the column structure here -- we just care about the editable 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.

This is a bit nitpicky, but perhaps naming it columnIsEditable is better, so that it's more clear that it's a boolean and not some sort of column object?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. Will update to isEditableTable and isEditableColumn

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 29, 2018 20:46 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 29, 2018 23:39 Inactive
Comment thread src/dash-table/Table.js
* Subscribe to [https://github.com/plotly/dash-table/issues/175](https://github.com/plotly/dash-table/issues/175)
* for more information.
* If the column-level `editable` flag is set it overrides
* the table-level `editable` flag for that column.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating documentation for table and column editable property

@valentijnnieman valentijnnieman 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.

One small suggestion, otherwise looks good to me!

editable: boolean,
column: IVisibleColumn
): boolean => editable && (column.editable === undefined || column.editable); No newline at end of file
editableColumn: boolean | undefined

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 a bit nitpicky, but perhaps naming it columnIsEditable is better, so that it's more clear that it's a boolean and not some sort of column object?

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 30, 2018 19:48 Inactive
@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 30, 2018 19:54 Inactive
isEditable(editable, columns[active_cell[1]]) &&
!isMetaKey(e.keyCode)
) {
// setProps({ is_focused: true });

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@valentijnnieman While merging, came across this little nugget of code that does nothing since the navigation work. Removing.

@chriddyp
chriddyp temporarily deployed to dash-table-review-pr-182 October 30, 2018 20:14 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

column level editable to take precendence over table level editable?

3 participants