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

Feature/route to string #306

Closed
wants to merge 9 commits into from
Closed

Feature/route to string #306

wants to merge 9 commits into from

Conversation

schovi
Copy link
Contributor

@schovi 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
Copy link
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
Copy link
Contributor Author

schovi commented Mar 15, 2013

@daffl Reverted commit, hope it is fine now.

@schovi
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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

@imjoshdean imjoshdean closed this Oct 10, 2013
@schovi
Copy link
Contributor Author

schovi commented Oct 10, 2013

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

@imjoshdean
Copy link
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

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

Successfully merging this pull request may close these issues.

None yet

4 participants