thoughtbot / carnival

An unobtrusive, developer-friendly way to add comments
MIT License
499 stars 30 forks source link

Move window.opener.postMessage to default-layout.coffee #198

Closed pbrisbin closed 9 years ago

pbrisbin commented 9 years ago

Now that visitors may be browsing Carnival itself, the "Ultimate Destination" feature could kick in, meaning that users aren't necessarily brought to the homepage after logging in.

When embedded, if a login doesn't bring the user to the homepage, the child window opened for login won't be closed. Moving the window.opener.postMessage call to occur on every page ensures that if there is a window.opener, it will always receive the message.

jferris commented 9 years ago

Is it a smell that we don't know where they'll end up in the embedded login window? Can't we send it with a specific value in the redirect_url parameter so that it always goes to the right place?

pbrisbin commented 9 years ago

Can't we send it with a specific value in the redirect_url parameter so that it always goes to the right place

We could do something like that. We could also send Referrer, though that's kind of a workaround still.

pbrisbin commented 9 years ago

So we can't actually use a redirect_url param here, because we don't own the handler for /auth/github/forward which is what the embedded login invokes. I wouldn't say impossible, but I can't really picture how to make something like that work and I'm sure it'd take a bit of machinery. I vote we either abuse Referrer or do what I'm doing here.

Thoughts?

jferris commented 9 years ago

I think this is probably fine. We may think of something better once we're more familiar with Yesod's auth.

pbrisbin commented 9 years ago

Also, once we move to more than just GitHub, we'll be sending users to the login route, not directly to GitHub's forward URL -- at that point we can handle a redirect_url parameter in loginHandler if we want. I'm going to merge this for now though.