Skip to content

Add an option to allow cells to be editable. #155

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

Closed
wants to merge 2 commits into from
Closed

Add an option to allow cells to be editable. #155

wants to merge 2 commits into from

Conversation

rht
Copy link

@rht rht commented Oct 23, 2014

No description provided.

@rht
Copy link
Author

rht commented Oct 24, 2014

Rationale:
I know I can simple overwrite _cellWriter, but having a native option is better.

This will ease the writing process of a dynamic editable table, for example:

var dynatable = $("table.dynatable").dynatable({dataset: {records: records }}).data('dynatable');
// ...
// some code
// ...

function updateDynaTable() {
    var newrecords = dynatable.records.getFromTable();
    // clean up the content of newrecords
    // ...
    // replace old records
    dynatable.settings.dataset.originalRecords = newrecords;
    dynatable.process();
}


// allow cells to be editable
if (column.contentEditable) {
td += ' contenteditable="true"';
Copy link
Member

Choose a reason for hiding this comment

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

This should be contentEditable="true" for the td attribute.

Also, is setting the column header to contentEditable the best way to allow this feature on columns? Wouldn't the more common use-case be that you want only the cells to be contentEditable and not the header row?

Copy link
Author

Choose a reason for hiding this comment

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

The td is created as a string, so this is the only way to append the attribute.

That's true. One approach is to use a custom class editable/columnEditable as a check instead?

Copy link
Member

Choose a reason for hiding this comment

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

For the attribute name, I was referring to the fact that you have contenteditable instead of contentEditable (capitalized "Editable").

Yeah, I'm thinking as a data attribute, like data-contenteditable or maybe namespaced to avoid conflicts with other libraries like data-dynatable-contenteditable on the th element in the column header cell.

Copy link
Author

Choose a reason for hiding this comment

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

Was aware that I didn't use the camelCase; I should have mentioned this earlier.
Though it isn't meant to be contentEditable http://www.w3.org/TR/2008/WD-html5-20080610/editing.html#contenteditable0 ?

(I can't edit this PR that since I removed the branch a long time ago...making a new one)

@rht
Copy link
Author

rht commented Nov 21, 2015

(here goes) Dup of #229

@rht rht closed this Nov 21, 2015
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.

2 participants