Skip to content
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

@anandaroop: Add about/* to responsive pages #838

Merged
merged 1 commit into from Feb 10, 2017
Merged

@anandaroop: Add about/* to responsive pages #838

merged 1 commit into from Feb 10, 2017

Conversation

craigspaeth
Copy link
Contributor

Solves artsy/force-merge#21

We inspect the routing stack of this redirect_mobile middleware in force-merge to see if we should route the request to the Force or MG app. Currently when MG 404s we embed the Force view in it, so these responsive about/* pages worked by falling through to that, but we should really explicitly define it here.

@anandaroop
Copy link
Member

Thanks for the explanation! 👍

Couple questions...

  • The About page in force doesn't appear to actually use a responsive layout. So this solution will prevent a 404 to a nonexistent MG page, but won't render a responsive About page, is that right?

  • Also, does this explain how the error was dependent on user login state? Or was that a red herring in the original issue?

@craigspaeth
Copy link
Contributor Author

Since the route is /about/* and not /about* it'll match all of the sub-routes of the about page, leaving the root /about route to render desktop mode. Unfortunately, this is fine b/c we don't actually have a mobile version of the about page today 😐

image

As for logged in vs. logged out state—that is strange! It appears that prior to this PR, mobile mode is trying to embed the desktop view which I could see how that might cause some funkiness as it attempts to pass the access token through to accomodate logged in states. Very much looking forward to killing this hack post-merger.

@anandaroop
Copy link
Member

Sounds good — thanks! Merging…

@anandaroop anandaroop merged commit 3ad9ab5 into artsy:master Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants