Skip to content

Instantly share code, notes, and snippets.

@iammerrick
Last active July 21, 2016 21:33
Show Gist options
  • Save iammerrick/fe0a83198a91788b5009344a4379cdfb to your computer and use it in GitHub Desktop.
Save iammerrick/fe0a83198a91788b5009344a4379cdfb to your computer and use it in GitHub Desktop.
  1. Since you are allowing passing in arbitrary children you can't bail out of shouldComponentUpdate without breaking expectations. You can keep a reference to current/old node to decide if you need to reinstantiate the widget.
  2. Use default props, they simplify your code a lot.
  3. Use PropTypes it will make consuming your code a lot easier.
  4. componentDidUpdate is often what you want for invoking methods on DOM related elements see https://facebook.github.io/react/blog/2016/01/08/A-implies-B-does-not-imply-B-implies-A.html
  5. If you can avoid letting them pass their own children you could drop the ReactDOM and use callback refs.
  6. The current implementation will end up invoking way to many calls on Kendo. Rather than calling all the hooks for every prop on update, see what changed and just invoke what you need to.
  7. Leaking the fact that this is a kendo widget underneathe will create some weird APIs. For example in React we make use of their event system, which allows a user to set events as attributes. Using your implementation here I have to provide a kendo specific object. It'll work but it's a little gross considering kendo is in the business of providing first-class UI widgets.
  8. A switched a lot of your code to ES2015, I noticed you were using some features but not others (arrows, classes, const).
import $ from 'jquery';
import kuidropdown from 'kendo-ui-core/js/kendo.dropdownlist.js';
import React from 'react';
import ReactDOM from 'react-dom';
class KendoDropDownList extends React.Component {
componentDidMount() {
this.synchronize();
}
componentDidUpdate(prevProps) {
if (/* prevProps !== this.props some use scalar equals, others deep equals, other reference checks */) {
this.synchronize();
}
}
componentWillUnmount() {
this.widget.destroy();
}
setup() {
const currentNode = ReactDOM.findDOMNode(this);
// Since we are allowing them to provide arbitrary children we need to reinstantiate kendo when the node changes.
if (currentNode === this.node) return;
this.widget.destroy();
const $element = $(currentNode);
$element.kendoDropDownList(this.props.options);
this.widget = $element.getKendoDropDownList();
this.node = currentNode;
}
update() {
// The problem with this implementation is you will triggering all events, etc *every prop change* so the work of figuring out what changed and calling the
// underlying widget on just the delta is what you want to do. I have not done that here.
this.bindEventsToKendoWidget(this.props.events);
this.callKendoWidgetMethods(this.props.methods);
this.triggerKendoWidgetEvents(this.props.triggerEvents);
this.unbindEventsToKendoWidget(this.props.unbindEvents);
}
synchronize() {
this.setup();
this.update();
}
triggerKendoWidgetEvents(events) {
events.forEach((event) => {
this.widget.trigger(event);
});
}
bindEventsToKendoWidget(events) {
Object.keys(events).forEach((event) => {
this.widget.bind(event, events[event]);
});
}
unbindEventsToKendoWidget(events) {
events.forEach((event) => {
this.widget.unbind(event);
});
}
callKendoWidgetMethods(methods) {
Object.keys(methods).forEach((method) => {
this.widget[method](...methods[method]);
});
}
// We always need to render since we aren't enforcing certain children returning false from
// shouldComponentUpdate would cause the children to not be updated.
render() {
return React.Children.only(this.props.children);
}
}
KendoDropDownList.defaultProps = {
children: <div />,
events: [],
methods: {},
triggerEvents: {},
unbindEvents: []
};
KendoDropDownList.PropTypes = {
children: React.PropTypes.element.isRequired,
events: React.PropTypes.array,
methods: React.PropTypes.object,
triggerEvents: React.PropTypes.object,
unbindEvents: React.PropTypes.array
};
export default KendoDropDownList;
@codylindley
Copy link

codylindley commented Jul 13, 2016

Thanks Merrick for the collaboration on this. Really appreciate the effort and that you are making me work for my perspective on the matter. Responses in BOLD.

(Anyone reading this should reference my original code here: https://github.com/kendo-labs/kendo-ui-boilerplates/blob/master/core-jquery-react-webpack-es6/src/kendoDropDownList.js)

Since you are allowing passing in arbitrary children you can't bail out of shouldComponentUpdate without breaking expectations. You can keep a reference to current/old node to decide if you need to re-instantiate the widget.

I'm trying to avoid the widget being re-created. I want the widget to be used as the user thinks the widget should be based on the docs. The way the pattern fits, is the React developer will have to update props alone on state changes from above. This will then update the widget, via the widget's api/interface. I realize this is not React'ish. But I'm providing a bandaid/stopgap

Use default props, they simplify your code a lot. Use PropTypes it will make consuming your code a lot easier.

Yes. Agree. Just hand not optimized yet.

componentDidUpdate is often what you want for invoking methods on DOM related elements see https://facebook.github.io/react/blog/2016/01/08/A-implies-B-does-not-imply-B-implies-A.html

If was trying favoring the React way alone, I would agree, but I have to think beyond that in this case.

If you can avoid letting them pass their own children you could drop the ReactDOM and use callback refs.

I want them to be able to pass or not. Just like with jQuery. Call it on a specific DOM element or not. Add children or not. But this follows the line of thinking that has the widget itself instantiated once and then update via the KUI api not the React api.

The current implementation will end up invoking way to many calls on Kendo. Rather than calling all the hooks for every prop on update, see what changed and just invoke what you need to.

I'm not clear what your issue is here. While I have not coded it yet, my intention was to diff the props and only make changes to the widget that have actually changed (using. componentWillReceiveProps). The componentDidMount is only even run once. Right? After that state changes from above will call the componentWillReceiveProps, which is where I determine if anything at all should happen based on diffs.

Leaking the fact that this is a kendo widget underneath will create some weird APIs. For example in React we make use of their event system, which allows a user to set events as attributes. Using your implementation here I have to provide a kendo specific object. It'll work but it's a little gross considering kendo is in the business of providing first-class UI widgets.

We are working on it. Real KUI for React components (https://github.com/telerik?utf8=%E2%9C%93&query=react). Until then I am trying to provide a wrapper pattern that sits between both worlds. Not just one. This is just something I'M (not telerik, or telerik engineers) trying to do to help developers with a stopgap issue. Savvy?

A switched a lot of your code to ES2015, I noticed you were using some features but not others (arrows, classes, const).

My es* usage is, strategic usage, based on our customers and neutral perspective/opinion on language advances)

@codylindley
Copy link

codylindley commented Jul 13, 2016

I did try your way. But so many issues occur once you try and remove and re-add the widget. I think the best path forward is the children of the wrapper are verboten from change after first mount. After that you have to change the widget via the non-react api (i.e. describe the change using props).

Final here, for now: https://github.com/kendo-labs/kendo-ui-boilerplates/blob/master/core-jquery-react-webpack-es6/src/kendoDropDownList.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment