Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Attaches row to DOM after all columns have been added #1863

Conversation

cmcculloh-kr
Copy link

@cmcculloh-kr cmcculloh-kr commented Sep 8, 2016

fixes #1850

JIRA ticket: https://jira.exacttarget.com:8443/browse/FUELUX-252

Because the columns need to actually be attached to the DOM at the time list_columnRendered is called, I broke that portion out of the renderColumn function and placed it in its own separate loop (which is only run if there is actually a list_columnRendered function to be called). I believe Automation Studio in particular uses (or used) this function to measure the size of a cell and then do something with that information (can't remember what).

Makes the repeater.js and repeater-list.js code (and related test files) conform to our current eslint standards across Fuel UX properties to try and increase maintainability (since these two areas in the code in particular have been notoriously hard to comprehend and work with).

There's not really any good way to unit test this changes since it is merely a speed improvement. I tested how long it took the repeater to render in firefox and chrome before and after the change. You can see the results of running the test 10x in each below:

All results are reported in units of seconds.

In Chrome:
Without changes:
(0.827 + 0.729 + 0.779 + 0.668 + 0.686 + 0.689 + 0.683 + 0.744 + 0.677 + 0.698) / 10
0.718 seconds on average

With changes (did it twice because I was surprised it didn't really make a difference, second set was even worse somehow):
(0.688 + 0.759 + 0.694 + 0.709 + 0.691 + 0.738 + 0.671 + 0.812 + 0.724 + 0.676) / 10
0.716 average
(0.721 + 0.744 + 0.703 + 0.697 + 0.71 + 0.805 + 0.684 + 0.722 + 0.751 + 0.764) / 10
0.7301 seconds on average

In Firefox:
Without changes:
(0.979 + 0.925 + 0.911 + 0.922 + 0.901 + 0.915 + 0.889 + 0.938 + 0.891 + 0.964) / 10
0.9235 seconds on average

With changes:
(0.905 + 0.912 + 0.869 + 0.927 + 0.946 + 0.973 + 0.905 + 0.846 + 0.913 + 0.964) / 10
0.916 seconds on average

What I believe is happening here is that the Unit Tests run against a shadow DOM instead of an actual rendered DOM. Since this change only speeds up things if it is running against an actual rendered DOM the unit tests will not help us here unless they are changed to render the repeater out to the actual page.

I created a test/repeater.html that would, using the data the costumer provided in #1850, render the repeater to the page and measure how long the render took. From the results you can see that there is a modest performance gain to be had with these changes (more pronounced in Firefox):

Rendered DOM
Chrome:
Without Changes:
(1.624 + 1.651 + 1.858 + 1.724 + 1.834 + 1.722 + 1.675 + 1.665 + 1.717 + 1.734) / 10
1.7204 seconds on average

With Changes:
(1.662 + 1.691 + 1.621 + 1.611 + 1.711 + 1.649 + 1.66 + 1.695 + 1.65 + 1.655) / 10
1.6605 seconds on average

Firefox:
Without Changes:
(1.887 + 1.686 + 1.796 + 1.703 + 1.796 + 1.77 + 1.817 + 1.797 + 1.818 + 1.784) / 10
1.7854 seconds on average

With Changes:
(1.535 + 1.442 + 1.43 + 1.436 + 1.496 + 1.505 + 1.437 + 1.48 + 1.426 + 1.45) / 10
1.4637 seconds on average

To test:

  • Check this branch out
  • Reset to commit acbc3eb
  • grunt servefast
  • Open http://localhost:8000/test/repeater.html
  • Open the console and observe the time it took to render (displayed something like "1.785 + ")
  • Reset to commit 1c4bb0e
  • grunt servefast
  • Open http://localhost:8000/test/repeater.html
  • Open the console and observe the time it took to render (displayed something like "1.785 + ")
  • Confirm that the render is, on average, faster with the changes applied
  • Run unit tests
  • Review code changes

@cmcculloh-kr cmcculloh-kr force-pushed the GH1850---adds-list_renderrowaftercolumns-option branch 2 times, most recently from ac3e508 to 82c7a5b Compare September 8, 2016 21:26
@cmcculloh-kr cmcculloh-kr force-pushed the GH1850---adds-list_renderrowaftercolumns-option branch from 82c7a5b to 7c7b633 Compare September 8, 2016 21:37
this.$primaryPaging.find('.combobox').combobox('disable');
this.$secondaryPaging.attr('disabled', 'disabled');
this.$prevBtn.attr('disabled', 'disabled');
this.$nextBtn.attr('disabled', 'disabled');
Copy link
Contributor

Choose a reason for hiding this comment

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

I think someone must have been worried about the extra keystrokes here. :-) Good call!

@interactivellama
Copy link
Contributor

I can confirm about a half second improvement in FireFox.

@interactivellama
Copy link
Contributor

Looks good to me. There are so many variations for Repeater, it's difficult to review all of them, but I've read every line, confirmed that this is faster and was unable to find any bugs in the repeater examples I have.

@cmcculloh-kr
Copy link
Author

Ball is back in your court @interactivellama

} else {
subset = [];
logWarn('WARNING: Repeater unable to find property to iterate renderItem on.');
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of break. I'm not sure I've seen one in the wild like this recently.

Copy link
Author

Choose a reason for hiding this comment

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

TBH the break was in the original code. I took it out for clarity somewhere along the way, and then once the code was simplified to this point I realized it was now actually less clear, and so I put it back in. :P

@interactivellama interactivellama merged commit 65d8b87 into ExactTarget:master Sep 13, 2016
@cmcculloh-kr cmcculloh-kr deleted the GH1850---adds-list_renderrowaftercolumns-option branch September 14, 2016 21:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants