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

Feature/route to string #306

Closed
wants to merge 9 commits into
from

Conversation

Projects
None yet
4 participants
@schovi
Contributor

schovi commented Mar 9, 2013

It is almost same pull request as #272 but the second one is from master branch, what is completely wrong (i have never done pull request before :) )

I improve the stringifiing part of this feature and add test for this. It is used in our current project for more than 2 weeks and it looks fine and working.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 15, 2013

Contributor

One request: Would it be possible to disable the auto code formatting? It's kind of hard to tell what actually changed if the diff shows all the tabs and spaces that got modified. I'll run everything through a beautifier for 1.2 so that things will be more consistent.

Contributor

daffl commented Mar 15, 2013

One request: Would it be possible to disable the auto code formatting? It's kind of hard to tell what actually changed if the diff shows all the tabs and spaces that got modified. I'll run everything through a beautifier for 1.2 so that things will be more consistent.

@schovi

This comment has been minimized.

Show comment
Hide comment
@schovi

schovi Mar 15, 2013

Contributor

@daffl Reverted commit, hope it is fine now.

Contributor

schovi commented Mar 15, 2013

@daffl Reverted commit, hope it is fine now.

@schovi

This comment has been minimized.

Show comment
Hide comment
@schovi

schovi Mar 20, 2013

Contributor

Had some issue with merging canjs master into this branch.
If this is problem, or there is lot of commits i can recreate it into only new one.

Contributor

schovi commented Mar 20, 2013

Had some issue with merging canjs master into this branch.
If this is problem, or there is lot of commits i can recreate it into only new one.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Mar 20, 2013

Contributor

Yes, sorry didn't have a chance to really look at this. Could you maybe also explain the reason for the string conversion a little more? I can see from the tests where this is going but am not sure why it would be necessary. Thanks!

Contributor

daffl commented Mar 20, 2013

Yes, sorry didn't have a chance to really look at this. Could you maybe also explain the reason for the string conversion a little more? I can see from the tests where this is going but am not sure why it would be necessary. Thanks!

@schovi

This comment has been minimized.

Show comment
Hide comment
@schovi

schovi Mar 20, 2013

Contributor

I noticed this as problem in my app, where i keep current state of page in hash params. Most of them are key - value where value is integer (id of something).
I think this is pretty common usecase and it works fine until

  1. I use strict comparsion (for example in view)
<% something.each(function(smth) { %>
    <div class="<% can.route.attr('something_id') === smth.id ? "active" : "" %>" <%= (el) -> el.data('smth', smth) %>>
        <%= smth.name %>
    </div>
<% }) %>

Then in control

{
    init: function() {},
    "div.click": function(el, ev) {
        can.route.attr('something_id', el.data('smth').id)
    },
    "{can.route} something_id": function(route, ev, what, somethingId) {
        if(somethingId) {
            $.ajax('something/' + somethingId)
        }
    }
}

So this is usecase. When i have app in zero state and click on something div it sets something_id = 1 (integer) in can.route.
It trigger binding, div will be active and some ajax call is started.
It is perfect.

But what happend when i do refresh with something_id parameter sets.
It parses route and sets parameters into can.route. In this case something_id = "1" (string). It triggers and call ajax request, but div wont be active because I compare it with 1 (integer) via ===
Now I can click on something div (with same id) and something_id = 1 (integer) is set on can.route. Div is mark as active and SECOND ajax call is called.

Contributor

schovi commented Mar 20, 2013

I noticed this as problem in my app, where i keep current state of page in hash params. Most of them are key - value where value is integer (id of something).
I think this is pretty common usecase and it works fine until

  1. I use strict comparsion (for example in view)
<% something.each(function(smth) { %>
    <div class="<% can.route.attr('something_id') === smth.id ? "active" : "" %>" <%= (el) -> el.data('smth', smth) %>>
        <%= smth.name %>
    </div>
<% }) %>

Then in control

{
    init: function() {},
    "div.click": function(el, ev) {
        can.route.attr('something_id', el.data('smth').id)
    },
    "{can.route} something_id": function(route, ev, what, somethingId) {
        if(somethingId) {
            $.ajax('something/' + somethingId)
        }
    }
}

So this is usecase. When i have app in zero state and click on something div it sets something_id = 1 (integer) in can.route.
It trigger binding, div will be active and some ajax call is started.
It is perfect.

But what happend when i do refresh with something_id parameter sets.
It parses route and sets parameters into can.route. In this case something_id = "1" (string). It triggers and call ajax request, but div wont be active because I compare it with 1 (integer) via ===
Now I can click on something div (with same id) and something_id = 1 (integer) is set on can.route. Div is mark as active and SECOND ajax call is called.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 20, 2013

Contributor

This is a good idea. We should review this and get it in for 1.2

Contributor

justinbmeyer commented Sep 20, 2013

This is a good idea. We should review this and get it in for 1.2

@ghost ghost assigned justinbmeyer Sep 20, 2013

@ghost ghost assigned imjoshdean Oct 10, 2013

imjoshdean added a commit that referenced this pull request Oct 10, 2013

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 10, 2013

Contributor

Couldn't merge, but implemented your suggestion, worked like a charm. Closing up on this.

Contributor

imjoshdean commented Oct 10, 2013

Couldn't merge, but implemented your suggestion, worked like a charm. Closing up on this.

@imjoshdean imjoshdean closed this Oct 10, 2013

@schovi

This comment has been minimized.

Show comment
Hide comment
@schovi

schovi Oct 10, 2013

Contributor

Good to hear guys .)
Where can i find the commit?

Contributor

schovi commented Oct 10, 2013

Good to hear guys .)
Where can i find the commit?

@imjoshdean

This comment has been minimized.

Show comment
Hide comment
@imjoshdean

imjoshdean Oct 10, 2013

Contributor

@schovi It's on our dev branch and will make it's way to the 1.2/2.0 release of CanJS.

Commit is here: b363ee9

Contributor

imjoshdean commented Oct 10, 2013

@schovi It's on our dev branch and will make it's way to the 1.2/2.0 release of CanJS.

Commit is here: b363ee9

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