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

Problems using custom rowWriter with Markup.js, content ends up outside <tbody></tbody> #43

Closed
flexd opened this issue Jan 24, 2014 · 7 comments

Comments

@flexd
Copy link

flexd commented Jan 24, 2014

Hi there.

I am experiencing a very odd problem trying to use Markup.js in my custom rowWriter.
My code works fine when just returning a string ala.

return "<tr><td>" + record.ident + "</td><td>" + record.first_name + " " + record.last_name + "</td><td>" + record.open_incidents + " åpne saker</td><td><button data-id='" + record.id + "' class='process_button btn btn-success btn-xs'>Behandle</button></td></tr>";

Using the code above in a custom rowWriter, like this, https://gist.github.com/flexd/427ac875d96735f2711c

It looks like this,
without_templating

But, if I use Markup.js to generate the HTML string using the commented out code in the gist above, it looks like this:
with_templating

As you can see in the second screenshot, the row has now somehow ended up outside <tbody></tbody>

I am not sure why this happens, as you can see here https://gist.github.com/flexd/333768cabcb5f7462ae2
Both methods return proper HTML, the only difference is that the template is formatted to be readable.

Looking at the default rowWriter and the source code, I have not yet found out what may be causing this, but since Markup.js seems to return a perfectly good HTML string I believe the problem exists within dynatable at the moment, I may of course be wrong.

https://github.com/alfajango/jquery-dynatable/blob/master/jquery.dynatable.js#L373

I am on IRC for the next 4-5 hours today and a while during the day tomorrow (CET). :-)

@JangoSteve
Copy link
Member

Interesting. Actually, now that I'm looking back at the code that appends the concatenated html string for the rows, it's pretty simple, but I'm wondering how it worked in the first place.

All dynatable does is calls the rowWriter function once for each row and string-concatenates the return values (that's all that's happening on the line you linked).

Then, it removes all rows and appends the new concatenated rows string (lines 424-425):

      obj.$element.find(settings.table.bodyRowSelector).remove();
      obj.$element.append(rows);

Where bodyRowSelector = 'tbody tr' by default. What's interesting is that we're appending the rows string to the element directly instead of to its <tbody>. So, I'm wondering if jQuery has some magic that knows when you're appending a <tr>...</tr> string to a table that has a <tbody> to instead append it to the tbody, which prevented this from coming to light before.

If that's the case, then there must be something about the formatting of the Markup.js output string that doesn't trigger this magic in jQuery.

I'm looking into this now to see if it can be easily fixed without causing the need for extra configuration when using dynatable on something other than a table (e.g. stylized list, etc.).

@JangoSteve
Copy link
Member

FYI, I was able to reproduce this on JSFiddle without dynatable, just by appending the two strings from your gist to two tables with jQuery and looking at the resulting HTML.

http://jsfiddle.net/Urdv7/

@JangoSteve
Copy link
Member

And, it looks like it's that initial \n followed by a bunch of spaced before the first <tr> that's causing it to not be appended to the tbody, as it's fixed in this update by getting rid of those and leaving everything else the same:

http://jsfiddle.net/Urdv7/2/

@JangoSteve
Copy link
Member

Can you try the updated branch above and see if that fixes the issue for you?

@flexd
Copy link
Author

flexd commented Jan 24, 2014

Thanks for the quick reply, that fixes the issue!

@brandondrew
Copy link

Shouldn't this be closed?

@flexd
Copy link
Author

flexd commented Feb 7, 2014

Yes, good point.

@flexd flexd closed this as completed Feb 7, 2014
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

3 participants