Skip to content

Conversation

simenbrekken
Copy link
Contributor

This patch resolves a small issue where ActiveDelegate.paramsAreActive and ActiveDelegate.queryIsActive only cast the incoming parameters to strings.

This prevents the following link from having the active class set:

<Link params={{numeric: 123}} />

@mjackson
Copy link
Member

Thanks @sbrekken. AFAICT this should only be necessary when the user is modifying activeParams and/or activeQuery directly because when we parse them from the URL they should always be strings. Is that what you're doing?

In any case, I've thought about this before and I think it makes sense to just use == instead of === here. Then we let JS do the casting for us. Thoughts?

@simenbrekken
Copy link
Contributor Author

@mjackson We're constructing our overly complicated URLs from #221:

var params = {
  productId: product.id,
  variantId: product.variants[0].id
  ...
}

I'll update my PR and let JS do the casting.

@mjackson
Copy link
Member

This looks good, thanks! Can you please squash these into a single commit?

@simenbrekken simenbrekken force-pushed the numeric-params-and-query branch from a1c9313 to b3c3e5a Compare September 17, 2014 14:39
@simenbrekken
Copy link
Contributor Author

@mjackson Done!

P.S: When you say squash these into a single commit, is the right way of doing this a forced push?

@mjackson
Copy link
Member

@sbrekken Thanks! And yes, when using GitHub you can just push -f to your branch and the PR will update automatically.

mjackson added a commit that referenced this pull request Sep 17, 2014
[fixed] Properly cast active params and query before comparing.
@mjackson mjackson merged commit 5ff6914 into remix-run:master Sep 17, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants