8
\$\begingroup\$

I am using Telerik's Kendo UI suite for a project, and was excited when they announced compatibility with Angular, but as I got a lot of code written I realized that they hadn't yet made the widget directives work with real JSON data - it only bound and talked to bare bones primitives. This was an undesired behavior, and in talking to their technical support they have opened an official GitHub ticket to resolve it.

But in the meantime, I still have to get work done. So I thought this would be a good place to try my very first angular directive. So, this is what I came up with:

Plunkr

The actual directive is in the k-data-bind.js file. I will provide any other information that is requested.

I am putting this on code review for 3 reasons:

  1. I am really bad at JavaScript, so I am absolutely sure that this code is terrible.
  2. I know very little about Kendo UI, I'm pretty certain that this approach has a ton of pitfalls and unexpected side effects. I am hoping a more intelligent developer can help me spot a few of them.
  3. I am hoping that someone can see some ways I could at least make this slightly more bulletproof while I wait for an official fix.

My approach to this was as follows:

  • I wanted the directive to be very simple, and just declare a model property to bind to.
  • I wanted the directive to be consistent between each widget
  • I wanted to need minimal ad-hoc code to handle the standard behavior. In short, I wanted this to work like the Kendo MVVM data-bind functionality.

To briefly run through what this does (or what I think it does)...

  • When the page loads, kendo emits two events; kendoWidgetCreated and kendoRendered
  • I watch for these events, and run very brief custom code that will either set the widget's state to match that of existing data, or add a custom behavior to the change or related event that will set the specified $scope property to data that the widget is changed to represent.

Example of usage

   <h1>Kendo MultiSelect</h1>
    <select kendo-multi-select
            k-data-text-field="'Name'"
            k-data-value-field="'Id'"
            k-data-bind="Model.Multiselect" 
            k-options="options.multiselect"></select>
    <hr />

k-data-bind.js

(function () {
    /** 
     * a plugin for kendo that will bypass the k-model and k-ng-model to try and bind
     * more appropriately with real data, instead of the primitives that kendo usually
     * works with using angular.
     **/
    angular.module('kendo.directives')
        .directive("kDataBind", ["$parse",function ($parse) {
            return {
                restrict: "A",
                scope: false,
                link: function (scope, element, attributes, controller) {
                    var getter = $parse(attributes.kDataBind),
                        setter = getter.assign,
                        // the property for the kendo data value field, if it is given. this
                        // property is needed by some of the widgets so that it can compare selected
                        // information to that stored in the widget's bound data source, and pull full
                        // objects out without trouble.
                        property = $parse(attributes.kDataValueField)() || null,

                        // a function to fire when the widget changes, encapsulated here
                        // simply to provide more consistency.
                        onChange = function (t, f, event) {
                            t.bind(event || 'change', function (e) {
                                scope.$apply(function () {
                                    f(e);
                                });
                            });
                        },

                        // a function to fire when the model value changes, encapsulated here
                        // simply to provide more consistency.
                        onWatch = function (f) {
                            scope.$watch(getter, function (n, o) {
                                f(n, o); 
                            });
                        },

                        // a function to fire when the widget is finished rendering, encapsulated here
                        // simply to provide more consistency. This is the best place to cause the
                        // widget to 'default' to a state that mirrors the data it is bound to on the model
                        onRendered = function (f) {
                            scope.$on('kendoRendered', function () {
                                f();
                            });
                        };

                    scope.$on('kendoWidgetCreated', function (event, target) {
                        // first, make sure we're interacting with the actual widget we
                        // want to work on, and not a different one - since this event is
                        // raised for every kendo widget on the page
                        if ($(target.element)[0] === $(element)[0]) {
                            // determine behavior based on the specific widget. This is necessary because
                            // each widget may be different. For example, the kendoDropDownList accepts information
                            // using the .select(n) function, but the kendoMultiSelect only takes information using
                            // the .value(arr) function. 

                            // we are using onWatch and onChange purely for matters of consistency. The goal is to
                            // keep as much of the code re-usable as possible for the sake of trying to tighten the
                            // behaviors between each other. 
                            switch (target.options.name) {
                                case "DropDownList":
                                case "ComboBox":
                                    (function () {
                                        onWatch(function (n, o) {
                                            target.select(function (dataItem) {
                                                return dataItem[property] === n[property];
                                            });
                                        });
                                        onChange(target, function (e) {
                                            setter(scope, e.sender.dataItem().toJSON());
                                        });
                                    })();
                                    break;
                                case "MultiSelect":
                                    (function () {
                                        onWatch(function (n, o) {
                                            var values = $.map(getter(scope), function(val, i){
                                                return val[property];
                                            }); target.value(values);
                                        });
                                        onChange(target, function (e) {
                                            setter(scope, target.dataItems().slice(0));
                                        });
                                        onRendered(function () {
                                            var values = $.map(getter(scope), function (val, i) {
                                                return val[property];
                                            }); target.value(values);
                                        });
                                    })();
                                    break;
                                case "AutoComplete":
                                    (function () {
                                        onChange(target, function (e) {
                                            setter(scope, e.sender.dataItem(e.item.index()).toJSON());
                                        }, 'select');
                                    })();
                                    break;
                                case "DatePicker":
                                case "DateTimePicker":
                                case "TimePicker":
                                    (function () {
                                        onWatch(function (n, o) {
                                            target.value(kendo.parseDate(n));
                                        });
                                        onChange(target, function (e) {
                                            setter(scope, e.sender.value().toJSON());
                                        });
                                        onRendered(function () {
                                            target.value(kendo.parseDate(getter(scope)));
                                        });
                                    })();
                                    break;
                                case "Grid":
                                    (function () {
                                        onChange(target, function (e) {
                                            setter(scope, e.sender.dataItem(e.sender.select()).toJSON());
                                        });
                                    })();
                                    break;
                                case "ColorPalette":
                                case "MaskedTextBox":
                                case "NumericTextBox":
                                case "Slider":
                                case "Editor":
                                    (function () {
                                        onWatch(function (n, o) {
                                            target.value(n);
                                        });
                                        onChange(target, function (e) {
                                            setter(scope, e.sender.value());
                                        });
                                        onRendered(function () {
                                            target.value(getter(scope));
                                        });
                                    })();
                                    break;
                                case "ListView":
                                    (function () {
                                        onChange(target, function (e) {
                                            var data = target.dataSource.view(),
                                                selected = $.map(target.select(), function (item) {
                                                    return data[$(item).index()];
                                                });
                                            setter(scope, selected);
                                        });
                                    })();
                                    break;
                            }
                        }

                    });
                }
            }
        }]);
})();

Notes

  • I completely updated the entire directive with much cleaner code and a smoother way of operating, without the need for a name field on the directive! Also removed the need for a second "switch", and homogenized the behavior functions for cleanliness and clearer reading.

  • I actually didn't want to use a switch, but given the simple logic of what I was doing, it seemed appropriate. The self-invoking functions are mostly for cleanliness, as I'm just accustomed to wrapping anonymous functions like that.

\$\endgroup\$
1
\$\begingroup\$

A short review;

  • Far too many comments, keep it down to 1 line of comment per line of code at the most. If really you need more, put all the other comment at the top of your coding block, right now the interspersed comments are blocking the reading flow.
  • Nice use of 'switch' actually, one of the few times it is appropriate
  • This:

                            case "TimePicker":
                                (function () {
                                    onWatch(function (n, o) {
                                        target.value(kendo.parseDate(n));
                                    });
                                    onChange(target, function (e) {
                                        setter(scope, e.sender.value().toJSON());
                                    });
                                    onRendered(function () {
                                        target.value(kendo.parseDate(getter(scope)));
                                    });
                                })();
                                break;
    

    is a little icky. I would define a named function upfront and then just return the function in here. Anonymous functions should be avoided when reasonably possible, like here.

All in all this is very reasonable code, I suspect you are not that really bad at JavaScript ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.