Domain Management: Email: Select site redirects to manage domains #3324
Conversation
umurkontaci
added
[Status] Needs Review
|
Seeing the following error when I go to
|
|
@breezyskies You're right, there was a slightly weird ES6 issue there. Should be fixed now. (Requires a full restart of the server) |
|
Working for me now. 👍 I'll leave the code review to @klimeryk. :) |
| module.exports = function() { | ||
| SiftScience.recordUser(); | ||
| function registerMultiPage( { routes, handlers } ) { | ||
| routes.forEach( path => page( path, ...handlers ) ); |
There was a problem hiding this comment.
It might be the lovely fewer I'm experiencing right now, but it took me a second to realize that path comes from iterating routes. I think it'd be better to keep the naming more uniform - iterating routes gives you a route, not a path. Or better yet go paths -> path, because that's usually how it's referenced.
|
Other than keeping the naming uniform, everything looks good. Actually, it looks great - awesome refactor! 👍 |
klimeryk
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
umurkontaci
force-pushed
the
fix/855-email-routing
branch
from
3214dd1 to
a662efb
Compare
umurkontaci
added
[Status] Needs Review
|
@klimeryk or @stephanethomas I've renamed it to |
|
LGTM 🚢 |
scruffian
added
[Status] Ready to Merge
and removed
[Status] Needs Review
Domain Management: Email: Select site redirects to manage domains
|
Nice, much simpler. Glad it worked well. |
This PR fixes #855: If you go to
/domains/manage/emailit will not redirect you to/domains/managebut will show all sites under/domains/manage/email, so that once you select a site, you will land on the email page.I also did some refactoring for the routes to reduce duplicate code.
/cc: @klimeryk for review
/cc2: @breezyskies for reporting the original issue and @mtias since he suggested the solution