Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support for Jquery plugins within ko bindingHandlers #1372

Closed
oniseijin opened this issue May 8, 2015 · 11 comments
Closed

Better support for Jquery plugins within ko bindingHandlers #1372

oniseijin opened this issue May 8, 2015 · 11 comments
Milestone

Comments

@oniseijin
Copy link
Contributor

Something prevents EasyTree JQuery plugin from functioning fully.

Problem: The knockoutjs binding, on the surface works, but is completely non-interactive. This is true for the work space version as well.

Static export done here: http://signaller-rat-84376.bitballoon.com/
-The top tree menu is done using a knockoutjs binding [NON-INTERACTIVE]
-The bottom using a standard binding to a div on the main index.html [WORKING](although, this approach does not work on the workbench because of div id creation/timing) workbench]
-workbench is also non-interactive with ko binding

So
A) why is the menu non-interactive within Bladerunner's standard framework
B) why is the "div" on the workbench, when set directly on the index.html of the workbench also not working (blank)

Using http://www.easyjstree.com/
HTML

<div id="kendoui.easytree.view-template">
        <div data-bind = "easyTree: treeMenuData"></div>
</div>

Binding

ko.bindingHandlers.easyTree = {
    init: function(element, valueAccessor, allBindings, data, context){
        var easytree = $(element).easytree({data: ko.unwrap(valueAccessor())})
        context.easytree = easytree

        console.log("easytree:" + JSON.stringify(context.easytree))
    }

}

this.treeMenuData is within the ViewModel as:

this.treeMenuData = ko.observableArray(
        [
            {
                "href":"http:\/\/www.easyjstree.com",
                "hrefTarget":"_blank",
                "text":"Home"
            },
            {
                "children":[
                    {
                        "href":"http:\/\/www.google.com",
                        "hrefTarget":"_blank",
                        "text":"Go to Google.com"
                    },
                    {
                        "href":"http:\/\/www.yahoo.com",
                        "hrefTarget":"_blank",
                        "text":"Go to Yahoo.com"
                    }
                ],
                "isActive":false,
                "isExpanded":true,
                "isFolder":true,
                "text":"Folder 1",
                "tooltip":"Bookmarks"
            },
            {
                "children":[
                    {
                        "text":"Sub Node 1"
                    },
                    {
                        "text":"Sub Node 2"
                    },
                    {
                        "text":"Sub Node 3"
                    }
                ],
                "isActive":false,
                "text":"Node 1"
            },
            {
                "text":"Node 2"
            }
        ]
    )

Work Bench Screen:
workbench

@dchambers
Copy link
Contributor

This is a bit hard to debug without access to working source code, so I took the code snippets you provided and wired them up, resulting in this easytree-spike repository. Everything worked immediately, but I'm guessing that if I do the same thing in a workbench then it won't work right?

@dchambers
Copy link
Contributor

Okay, I just created an easytree-brjs-spike repository which tries to run the same code in a BRJS workbench, and I can confirm that it's presently not working as you describe...

@oniseijin
Copy link
Contributor Author

Thanks for the confirmation. It should work: but it is not interactive, with no errors.
I did get jsTree working though, in a similar manner.

There are a few other libraries I'm seeing how well they integrate as well: I'll raise separate issues, but there might be something common: e.g. #1374

I can put the code up if there is trouble reproducing.

@dchambers
Copy link
Contributor

Hi @oniseijin, now that I have one example that's not working then I can debug that, and it's possible, like you say, that some of the other things you are seeing are related...

@dchambers
Copy link
Contributor

Okay, this is happening because the EasyTree plug-in expects the tree to exist within the global document, for example when it attempts to register the click listeners here:

$($this.selector + " .easytree-node").on("click", nodes, nodeClick);
$($this.selector + " .easytree-expander").on("click", nodes, toggleNodeEvt);
$($this.selector + " .easytree-icon").on("dblclick", nodes, toggleNodeEvt);
$($this.selector + " .easytree-title").on("dblclick", nodes, toggleNodeEvt);

whereas the knockout adaptor does actually make available a reference to the element within the init() method, here:

ko.bindingHandlers.easyTree = {
  init: function(element, valueAccessor, allBindings, data, context){
    var easytree = $(element).easytree({data: ko.unwrap(valueAccessor())})
    context.easytree = easytree

    console.log("easytree:" + JSON.stringify(context.easytree))
  }
}

The simple knockout page I created worked because the template being rendered into was already added to the document when ko.applyBindings(viewModel) was invoked, whereas in BRJS the template gets added later, and the ko.applyBindings(viewModel, templateElem) form of the function is used instead.

I'm not quite sure what the right way to fix this is yet...

@dchambers
Copy link
Contributor

@oniseijin, I've been thinking about this, and I think that the fundamental problem is that the EasyTree plug-in has not been designed as an isolated component, and that this isn't something that can easily be worked around.

So far we've seen that it can't handle dynamically adding tree elements to an existing page, but even if we changed our APIs so that worked, then we'd get further problems like click listeners being added to tree elements that had already been added to a page.

To provide some background, the getElement() method of KnockoutComponent looks like this:

KnockoutComponent.prototype.getElement = function() {
  if (!this.m_bViewBound) {
    this.m_bViewBound = true;
    ko.applyBindings(this.m_oPresentationModel, this.m_eTemplate);
  }

  return this.m_eTemplate;
};

and we purposely apply the bindings before the element could ever be attached to the document, since it's more efficient to do it like that. As you might imagine, trying to change how this works might easily break backwards compatibility for existing apps, yet would not completely fix the underlying problems anyway.

I'd either suggest that you use a more component friendly tree implemenation, or that you try patching their code so that it no longer affects global page state.

@andyberry88 / @james-shaw-turner: do you agree with this?

@oniseijin
Copy link
Contributor Author

Not against using a different library if it goes completely against the general architectural principles, but as you've seen in some other examples, helping with the ordering of initialization may go a long way!

@dchambers
Copy link
Contributor

Yep, agreed. I just started looking at #1373 and can see that's also related to the same underlying issue, and #1374 looks like it may be the caused by the same thing too.

One thing I am noticing here is that you seem to be creating knockout bindings for all of the components on the page, whereas we build apps by composing a number of Component instances — that is, things that implement the Component interface.

In your app, for example, I would imagine you having the following components:

  • Tree Component (a wrapper that implements Component and integrates the underlying tree library)
  • Chart Component (a wrapper that implements Component and integrates the underlying chart library)
  • Form-Based Components (Knockout components that provide arbitrary functionality)

Because Component implementations have much richer life-cycle methods, we haven't run into the same problems you're seeing. Ocassionally, when we want to create Knockout components that wrap some form around another component (e.g. a tree) then we use the knockout component binding we have that allows you to embed one component within another.

So, although I'd agree that only initializing knockout plug-ins after the elements have been added to the page sounds like a very good thing to do, I suspect you won't actually be dependent on this fix if you build your apps in the same way we do.

@oniseijin
Copy link
Contributor Author

@dchambers : I see you have a proposed fix, fast work. As far as Component usage, I could not find any examples of using third party libraries that "renderTo" an element in the BRJS samples, but I saw a hint at https://www.caplin.com/developer/component/chart/getting-started/chart-Write-a-chart-component

Basically, creating an element within a PresenterComponent, or KnockoutComponent:
this._rootElement = document.createElement('div');

Then using that element to attach some third party plugin to it, and then finally attaching the Component itself to the main page... not sure I fully get how the Frame, and Components interact with each other in KnockoutComponent and PresenterComponent, but I will experiment a bit later.

I suppose a custom Component would then need to use the SimpleFrame directly to display, or simply attach the element itself without using a Frame

Update: gave this a try at #1374 -> okay as a starting point; the key is to attach the chart after the element is attached to body (much like what you are looking into for KnockoutComponent). Might extend SimpleFrame to call attachChart after it attaches the element.

@dchambers
Copy link
Contributor

Hi @oniseijin, I'm glad you found the Caplin docs since there is probably quite a bit of additional stuff there. However, as a word of warning, the Write a chart component documentation you linked to is still referring to the old Component interface rather than the new one.

Apart from that, I've left a comment on #1374 that also addresses some of the points you raise here.

@andy-berry-dev
Copy link
Member

This is fixed by #1379 which will be in the BRJS 1.0 release.

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

No branches or pull requests

5 participants