-
Notifications
You must be signed in to change notification settings - Fork 27.3k
feat($routeProvider): allow setting caseInsensitiveMatch on the provider #9873
feat($routeProvider): allow setting caseInsensitiveMatch on the provider #9873
Conversation
src/ngRoute/route.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will need to change slightly if #9731 lands, and I think we want to land it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, but it shouldn't be a big deal. I can rebase / change this one when #9731 lands. I would like to get this one in as I was really surprised that we can't have provider-level definition for case-sensitive routes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#9731 has landed, should rebase
4bdf4db to
ff13227
Compare
|
@caitp it was rebased on master after #9731landed. Ready for the review / merging. |
|
thanks, looking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should use if (!'prop' in routeCopy) instead, since that's really what we're testing. Either way, looks fine to me
|
@caitp thnx for the review. I guess based on your comments we could merge it but then we need to figure safe way of landing those types of things in 1.3.x / 1.4. Which goes back to the discussion we had yesterday. I guess this PR is a perfect example of little / low risk new features for which we need to figure out good strategy. |
|
well it's not really changing existing behaviour, it's adding a new behaviour --- so it should be pretty safe. |
|
@pkozlowski-opensource this looks good to me, merge at will |
Fixes #6477