Visibility button widget - share + code review!?!

2 posts, 0 answers
  1. Alexandra
    Alexandra avatar
    18 posts
    Member since:
    Sep 2015

    Posted 21 Aug 2017 Link to this post

    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

  2. Angel Petrov
    Admin
    Angel Petrov avatar
    1153 posts

    Posted 23 Aug 2017 Link to this post

    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.
Back to Top