-
Notifications
You must be signed in to change notification settings - Fork 1
Add Country id route and update routes to use countryId path param #210
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
Conversation
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.
Thanks for this @SakshiKekre. I have a few questions in here and mostly nits.
- Add loader validation for country routes - Redirect invalid countries to default (us) - Remove validation from useCurrentCountry hook - Update tests for new validation behavior
@anth-volk Thanks for the review. I have made changes based on your comments + bug fix (please see PR description for more). |
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.
Thanks for this @SakshiKekre. My main takeaway here: I really don't think we should be using loaders if we can avoid it. I think the code as it is is probably fine, but I wouldn't be surprised if down the road, we added something and literally rendered a white screen for a perceptible amount of time.
Skipped loader approach due to potential delayed rendering
Test malicious inputs including SQL injection, XSS, and path traversal
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.
Let's move with this. If we find ourselves adding too many components that don't fit inside the layout component we have, we may want to modify.
Fixes #196
Implemented country-based URL routing to support multi-country functionality:
[EDIT] Updates since last review
Fixed country routing validation issue
Issue/Bug: Invalid paths like /configurations were incorrectly accepted as country codes and router would not redirect/navigate to country path.
Changes made:
Technical details: