search

venantius / accountant

ClojureScript navigation for single-page applications, made simple.
Eclipse Public License 1.0
250 stars 39 forks source link

Sec 2 #18

Closed bago2k4 closed 8 years ago

bago2k4 commented 8 years ago

Update Accountant to use Secretary 2. As discussed here i finally had a response from the Secretary guys and i updated the library accordingly.

I have also improved the check on the clicked anchor: it was checking only the path, now i check for the domain to be empty or the same and if the target of the anchor is the same window, if not it shouldn't call e.preventDefault().

venantius commented 8 years ago

I'll have to review this in greater depth later this week. My gut says this is fine, but that I probably won't want to merge it into master; rather we'll either create an Accountant 0.2.0 branch or we'll refactor this code so that both Secretary 1 and Secretary 2 can be supported.

bago2k4 commented 8 years ago

@venantius thanks for the fast reply. Makes sense to me what you are saying since Secretary is doing the same, even if i don't like it, it would be better to create another repo/module IMHO. Sorry for the delay in opening this.

NB: i didn't mentioned this: if the anchor you click has no href set (checked with empty?) it calls only preventDefault but does nothing. I use this for anchors that have an onClick set and usually call preventDefault to prevent the navigation.

bago2k4 commented 8 years ago

@venantius bump :+1:

venantius commented 8 years ago

Sorry, I don't mean to ignore this. Bump is fair. Let me push up a v2 branch; I might ask you to then re-submit this PR against the v2 branch.

venantius commented 8 years ago

Okay; there's now a v2.0.0 branch; could you re-open this PR against that branch? Sorry for the hassle - I do want to get this merged.

venantius commented 8 years ago

The motivation for this PR has been somewhat deprecated by @arohner's work generalizing Accountant to work with any routing library. I'm going to close this.