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

Fixed updateCell to work correctly with child rows #1381

Merged
merged 2 commits into from
Apr 12, 2017

Conversation

andysleigh
Copy link
Contributor

The updateCell method currently fails on tables with child rows. This is because when it gets the index of the row it is searching all rows (including child rows). The resulting index is therefore not the correct index to be using against the cache. Also when the update row is inserted into the cache it no longer has the child row linked to it.

The fix therefore has two parts. The first is to filter the list of rows to exclude child rows when finding the row index. The second part is to ensure the child row is linked to the updated row before inserting it to the cache.

@Mottie
Copy link
Owner

Mottie commented Apr 7, 2017

Hi @andysleigh!

Thanks for catching this problem and providing a fix!

@@ -1328,6 +1328,11 @@
cache[ c.columns ].raw[ icell ] = tmp;
tmp = ts.getParsedText( c, cell, icell, tmp );
cache[ icell ] = tmp; // parsed
var tmpRow = cache[c.columns].$row;
if (tmpRow.length == 2) {
Copy link
Owner

Choose a reason for hiding this comment

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

Please change this to if (tmpRow.length > 1) { as it is possible to have more than one child row.

@Mottie
Copy link
Owner

Mottie commented Apr 7, 2017

Oh and please don't include the dist folder files... it takes longer to scroll through the diff

var tmpRow = cache[c.columns].$row;
if (tmpRow.length == 2) {
// reapply child row
$row = $row.add(tmpRow[1]);
Copy link
Owner

Choose a reason for hiding this comment

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

And as above, more than one child row is possible - $row = $row.add(tmpRow);

That "should" work, but I haven't tested it... you should be able to add all the rows back; jQuery will prevent duplication.

cache[ c.columns ].$row doesnt need to be updated in updateCell.
@Mottie
Copy link
Owner

Mottie commented Apr 12, 2017

Oh, I was thinking there was more to be done for this PR... so essentially only two changes were made:

  • Add .not( '.' + c.cssChildRow ) to find the correct row index.
  • Remove the line with cache[ c.columns ].$row = $row;.

Is that correct?

@andysleigh
Copy link
Contributor Author

Yes this seems to be all it needed. The first change sorts out the indexing so we get the correct row from the cache. The second part, following on from my original change I realised there's no need to update cache[...].$row at all in this method so I removed it. Sorting now works correctly after calling updateCell and the child row remains linked to the parent row.

@Mottie
Copy link
Owner

Mottie commented Apr 12, 2017

Ok great, thanks again so much for your help!

@Mottie Mottie merged commit 1b87904 into Mottie:master Apr 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants