Fix routes reset namespace display#863
Conversation
RobbieTheWagner
left a comment
RobbieTheWagner
left a comment
There was a problem hiding this comment.
This looks great to me. Just had one comment.
Other than that, my only question is does this handle edge cases? For example if you had a root route of comment and a nested route of foo/bar/comments, both would be named just comments, so if you select the nested one, does it not select the root one as well?
| "PostRoute", | ||
| "application", | ||
| "application", | ||
| "comments", |
There was a problem hiding this comment.
Do we need "comments" twice here?
There was a problem hiding this comment.
@rwwagner90 I double checked and the test seems to want "comments" listed twice similar to "application" and "post". In order to keep from compromising the existing test case, I didn't want to modify it too much. However, I can reassess it if you recommend doing so.
| name: 'comments', | ||
| }, | ||
| message: 'resource match fails when current route does not match the root route', | ||
| }, |
There was a problem hiding this comment.
@rwwagner90 You are very right. I did run into this edge case. The above test two cases roughly model this scenario.
Edge case Failure
Adding the following URL check const hasMatchingURL = routeURL === currentRouteURL; resolved this edge case.
Edge Case Success
Please let me know if I failed to address your concern.
RobbieTheWagner
left a comment
RobbieTheWagner
left a comment
There was a problem hiding this comment.
LGTM, thanks so much!
* Implement bug fix * Post rebase refinements * Test case refinement
2 participants
Resolves: #832
