Add accept-language support #770
Conversation
|
|
||
| export default ( | ||
| <Route path="/" component={App}> | ||
| <Route path="/(:lang/)" component={App}> |
There was a problem hiding this comment.
This should probably be path="/(:lang/)firefox/" but that could wait if you'd like.
There was a problem hiding this comment.
I wonder how this will work if you get something like /firefox/addons/, would it be smart enough to see that there is no lang? Maybe we shouldn't worry about redirecting the lang other than at /?
There was a problem hiding this comment.
Yes this does work as long as the rest of the URL matches. See https://github.com/mozilla/addons-frontend/pull/770/files#diff-4f380033f80b73f222c107868068167aR72
|
r+wc |
|
@mstriemer I've added a new commit which removes the index route and redirects in development (for the disco pane). This makes it a bit more correct so hitting / will be a 404 which I think is right. I also moved from 301 to 302 for redirection which feels more appropriate. |
| } | ||
|
|
||
| if (appInstanceName === 'disco' && isDevelopment) { | ||
| app.get('/', (req, res) => |
There was a problem hiding this comment.
react-router has a <Redirect /> component, can you add that to the router in disco/routes.js when in development instead?
That would be nice for removing app specific code from the core apps. What I can't see is what status code you'll get using that. |
|
I suppose we could say for local-development we don't really care. |
|
Actually react-router won't perform the redirect on the server, so I'm not sure what will happen. Maybe it returns a |
Yeah - I can't seem to get it to work - presumably because you need to respond to a URL you don't care about in order to be running the JS which will do the location change :/ |
|
I'm going to land this as is, and we can look at improving the redirects stuff in due course. |
2 participants
Fixes mozilla/addons#9747