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

Adds list_renderRowAfterColumns option #1850

Closed

Conversation

aheile
Copy link

@aheile aheile commented Aug 4, 2016

Adds repeater-list option: list_renderRowAfterColumns to wait to append the
row until its columns have already been appended to it.
This helps with some Firefox unresponsive script warnings in trying to render our repeater.

repeater-list option: list_renderRowAfterColumns to wait to append the
row.
This helps with some Firefox unresponsive script warnings
@mtrepina
Copy link

@interactivellama Any possibility of prioritizing this PR. We are seeing some bad bad things with the repeater in firefox when 500+ cells are rendered.

@futuremint
Copy link
Contributor

@mtrepina @swilliamset is handling Fuel UX requests now, please ask him to prioritize reviewing this in their next sprint. Thanks.

@futuremint
Copy link
Contributor

Also, @aheile can you please add a unit test for this that validates this solves the slow script problem?

@aheile
Copy link
Author

aheile commented Aug 20, 2016

@swilliamset Here is a sample with lots of cells which averages 13-15 seconds for me in Firefox:
http://jsfiddle.net/qpyygcqw/

Verses 7-8 seconds:
http://jsfiddle.net/mfy29f41/

@cmcculloh-kr
Copy link

@aheile what is the purpose of adding this as an option instead of just moving where the row gets appended to the body? Basically: what are the potential adverse effects of moving $tbody.append($row); below the for loop and calling it a day?

@aheile
Copy link
Author

aheile commented Aug 22, 2016

@CormacMcCarthy I would be fine with that. I was just trying to minimize any potential negative impact that I might not be aware of.
I'll look to you for guidance. Thanks!

@cmcculloh-kr
Copy link

Reading through the code a bit further it does look like there may be some potential side effects of doing it this way. Specifically, right now viewOptions.list_columnRendered gets called each time a new cell is attached to a row which is currently already attached to the DOM. The new way would call list_columnRendered before the row (and therefore the cell) is attached to the DOM. I believe that Automation Studio either was or is using this function to adjust the size of their cells (or something like that). So, just moving the append would break that (because you couldn't properly measure the cell yet if it isn't attached to the DOM).

However, instead of passing in an option that has to be documented, etc, we should perhaps rip this portion out of the renderColumn method :

    if (!(columns[columnIndex].property === '@_CHECKBOX_@' || columns[columnIndex].property === '@_ACTIONS_@') && this.viewOptions.list_columnRendered) {
        this.viewOptions.list_columnRendered({
            container: $row,
            columnAttr: columns[columnIndex].property,
            item: $col,
            rowData: rows[rowIndex]
        }, function () {});
    }

and, if it is going to be necessary to call it, call it column by column after the row is appended to the DOM, roughly like this (this probably won't work, will need some adjustments):

    for (i = 0, length = this.list_columns.length; i < length; i++) {
        renderColumn.call(this, $row, rows, index, this.list_columns, i);
    }

    $tbody.append($row);

    if (!(columns[columnIndex].property === '@_CHECKBOX_@' || columns[columnIndex].property === '@_ACTIONS_@') && this.viewOptions.list_columnRendered) {
        for (i = 0, length = this.list_columns.length; i < length; i++) {
            this.viewOptions.list_columnRendered({
                container: $row,
                columnAttr: columns[columnIndex].property,
                item: $col,
                rowData: rows[rowIndex]
            }, function () {});
        }
    }

@cmcculloh-kr
Copy link

Closing in favor of #1863

@cmcculloh-kr cmcculloh-kr added this to the 3.15.7 milestone Nov 22, 2016
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.

4 participants