Skip to content

Routing function with prefixing - #15

Open
mberkom wants to merge 3 commits into
rniemeyer:masterfrom
mberkom:routing-func-with-prefixing
Open

Routing function with prefixing#15
mberkom wants to merge 3 commits into
rniemeyer:masterfrom
mberkom:routing-func-with-prefixing

Conversation

@mberkom

@mberkom mberkom commented Oct 12, 2012

Copy link
Copy Markdown
Contributor

I extended the getBindings method to call a getCleanPrefix function that recurses up the DOM tree looking for nodes with a data-class-prefix attribute and returning the complete prefix once it hits the <body> tag or an element with a data-class-prefix containing "$root".

The newly generated class prefix is then passed to the binding router and prepended to the class name before it attempts to look up the binding in the bindings object.

This allows you to write HTML like this:

<div data-class-prefix="$root.test">
    <div data-class-prefix="level1">
        <span data-class="test"></span>
        <span data-class="test2"></span>
    </div>
    <div data-class-prefix="level2">
        <span data-class="test"></span>
        <span data-class="test2"></span>
    </div>
</div>

instead of...

<div>
    <div>
        <span data-class="test.level1.test"></span>
        <span data-class="test.level1.test2"></span>
    </div>
    <div>
        <span data-class="test.level2.test"></span>
        <span data-class="test.level2.test2"></span>
    </div>
</div>

Technical Details

The getPrefix method decorates all child nodes of an element with a data-class-prefix attribute containing "$root". This is easiest to understand if you open up your inspector and look at the generated HTML. It adds the data-class-prefix attribute to all nodes, so that the second time a child element on the same branch goes to get its prefix, it doesn't have to recurse all the way up the tree again.

Knockout's ko.domData might be an effective way to clean things up, but I'm not sure it's the most straightforward thing to implement and might be a source of memory leaks if DOM elements are removed outside of Knockout.

Thoughts?

@rniemeyer

rniemeyer commented Oct 17, 2012

Copy link
Copy Markdown
Owner

@mberkom - still intend on looking through this soon. Thanks for submitting it.

@mberkom

mberkom commented Oct 17, 2012

Copy link
Copy Markdown
Contributor Author

Sounds good. I just squashed all the commits together to make things easier to follow.

@rniemeyer

rniemeyer commented Oct 31, 2012

Copy link
Copy Markdown
Owner

@mberkom Hi Michael- I took a look at this a bit last weekend. Pretty interesting. I think that I would probably use the ko.utils.domData functions and assume that Knockout will be cleaning up the elements. It seems to work well.

I think though to keep this library a bit simpler for the time being, I would like to hold off on merging this in, although I certainly see the benefits of this approach.

@mberkom

mberkom commented Nov 1, 2012

Copy link
Copy Markdown
Contributor Author

@rniemeyer Sounds good. I'm going to continue to use it in the project I'm working on. So far, the simple data attribute mechanism is working fine and doesn't annoy me much. It has the added benefit of being immediately obvious when you open up the inspector... However, I could see switching to ko.utils.domData in the future.

@mberkom

mberkom commented Nov 20, 2012

Copy link
Copy Markdown
Contributor Author

@rniemeyer Would it be possible to reconsider what it'd take to get this feature included officially? I've found it to be incredibly handy and easy to use. However, there are a couple of areas that could certainly use more thought.

  1. How the prefixes are formatted - While you can replace the routing function with any implementation you want, you're currently restricted to formatting your class prefixes with the dot notation.
  2. How the prefixes are stored on nodes - Adding data-class-prefix attributes on every node it recurses is a little messy looking in the inspector. However, there are downsides to switching completely to ko.utils.domData. You lose the ability to see the complete binding path in the inspector and also increase the likelihood of memory leaks if someone doesn't clean things up properly.

For most Knockout developers doing simple projects, this library is probably overkill. On the other hand, those of us doing complex projects need every last bit of organization & re-usability that we can get.

@mberkom
Disabled prefixing by default, using ko.utils.domData
93be5a6 
Prefixing is disabled by default and can be enabled via the options
hash. If enabled, it will only decorate nodes that have a `data-class`
attribute with a `data-class-prefix` attribute. Otherwise, it uses
`ko.utils.domData` to keep track of things.
@rniemeyer

rniemeyer commented Nov 21, 2012

Copy link
Copy Markdown
Owner

@mberkom - I am definitely willing to consider putting this type of functionality into the library. I do think that ko.utils.domData is a safe choice, given that KO will clean it up. However, since we only need to store a string, an attribute is also not a bad choice.

I will have to take another look through the code. It would be nice if we didn't need to have $root in the values.

@mberkom

mberkom commented Nov 21, 2012

Copy link
Copy Markdown
Contributor Author

@rniemeyer Cool! Make sure to look at my latest commit description. It explains what I did.

Yeah, I'm not a fan of having $root in there, but it really speeds things up because once one branch of the tree has been traversed all the leafs and branches coming off that branch no longer have to traverse all the way up to the trunk but can stop the first time they encounter a class prefix containing $root.

@mberkom
ko virtualElement fix for IE9 & IE10
6aa81d4 
The getPrefix method wasn't finding the parentElement property on the
virtual element node and so wasn't traversing up the DOM tree to find
the correct binding prefix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mberkom @rniemeyer