This is a migrated thread and some comments may be shown as answers.

Visibility button widget - share + code review!?!

1 Answer 53 Views
Grid
This is a migrated thread and some comments may be shown as answers.
Alexandra
Top achievements
Rank 1
Alexandra asked on 21 Aug 2017, 10:48 AM

Hi all,

I have create a new widget for a button to display the columns of a grid outside of the column menu that exist in the telerik framework. The code is based on columns menu functionality removing the sorting, filtering and mobile.

The button also implements the grid reorderable functionality not yet implemented in the telerik framework as this thread says:
http://www.telerik.com/forums/issue-on-column-menu-and-column-reorder-functionality

What I would require, if someone is kind enough, is a code review since I only started working with javascript recently:
Here is the code file and some explanations:

1. I have create a handler just like for columnShow/columnHide to attach to grid columnReorder

that._updateColumnsOrderStateHandler = proxy(that._updateColumnsOrderState, that);
that.owner.bind('columnReorder', that._updateColumnsOrderStateHandler);
...
if (that._updateColumnsOrderStateHandler) {
    that.owner.unbind('columnReorder', that._updateColumnsOrderStateHandler)
}

2. In the event I do these actions:
- determine the position of the reorder (this was done by changing the telerik framework in order to receive the position text: 'before' or 'after' - if anyone can determine this in a way that does not require to change the framework please let me know)
- reorder the menuitemcheckboxes
- update the checkbox column indexes

var before = e.newIndex < e.oldIndex;
if (e.position) {
    before = e.position === "before";
}
reorder(this.wrapper.find('input[' + fieldAttr + ']'), e.oldIndex, e.newIndex, before, 1);
selector = this.wrapper.find('input[' + fieldAttr + ']');
setTimeout(function () {
    updateColumnsIndex(selector);
}, 100);

3. I have adapted the reorder code from the framework with the actions:
- determine the destination menuitemcheckbox
- add in source the menuitemcheckbox(es)
- move the source before or after the destination based on position

function reorder(selector, source, dest, before, count) {
    var sourceIndex = source;
    source = $();
    count = count || 1;
    var d = findDestination(selector, dest, before);
    if (d.length === 0) {
        before = true;
        index = 1;
        while (d.length === 0) {
            d = findDestination(selector, index, before);
            index = index + 1;
        }
    }
    for (var idx = 0, destIndex = dest + (before ? -1 : 0) ; idx < count; idx++) {
        t = selector.filter(function () {
            return $(this).attr(indexAttr) == (sourceIndex + idx);
        }).closest('[role=\'menuitemcheckbox\']');
        source = source.add(t);
    }
    source[before ? 'insertBefore' : 'insertAfter'](d);
}

4. Since there can be columns that can not be shown or hide the determination of destination is done like:
- try to find the menuitemcheckbox based on the index attribute; if successfull return the found menuitemcheckbox
- if the destination is a column not on the menu based on position determine the smallest menuitemcheckbox with the index attribute greater than destination or the greatest menuitemcheckbox with the index attribute less than destination

function findDestination(selector, index, before) {
    var d = selector.filter(function () { return $(this).attr(indexAttr) == index; }).closest('[role=\'menuitemcheckbox\']');
    if (d.length !== 0)
        return d;
    col = null;
    if (before) {
        min = null;
        selector.each(function () {
            indexValue = parseInt($(this).attr(indexAttr));
            if (indexValue > index && (min === null || indexValue < min)) {
                min = indexValue;
                col = $(this);
            }
        });
    } else {
        max = -1;
        selector.each(function () {
            indexValue = parseInt($(this).attr(indexAttr));
            if (indexValue > max && indexValue < index) {
                max = indexValue;
                col = $(this);
            }
        });
    }
    if (col === null) {
        return $([]);
    }
    return col.closest('[role=\'menuitemcheckbox\']');
}

5. The update iterates over all menuitemcheckboxes and determines the column index based on the field attribute

function updateColumnsIndex(selector) {
    var columns = toHash(that._ownerColumns(), 'field');
    selector.each(function(){
        column = columns[$(this).attr(fieldAttr)];
        if (column) {
            $(this).attr(indexAttr, column.index);
        }
    });
}

 

Thank you,

Alexandra

1 Answer, 1 is accepted

Sort by
0
Angel Petrov
Telerik team
answered on 23 Aug 2017, 11:03 AM

Hi Alexandra,

Making a thorough code review would require some time and is out of our support scope. Nevertheless I have checked the code and it does have the general structure of a normal widget. Two things that I noticed were

1. Variables are declared when needed, maybe you can consider declaring them at the begining of a function.
2. Inside the _updateColumnsOrderState function a number of functions are declared. I would strongly suggest moving those out of the function scope.

The template definition at the bottom can also be moved to the top with the rest of the globals.

Other than the above if all functions correct you should not experience any problems using the widget.

Regards,

Angel Petrov
Progress Telerik
Try our brand new, jQuery-free Angular 2 components built from ground-up which deliver the business app essential building blocks - a grid component, data visualization (charts) and form elements.
Tags
Grid
Asked by
Alexandra
Top achievements
Rank 1
Answers by
Angel Petrov
Telerik team
Share this question
or