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

Layout: Replace LayoutLoggedOut and LayoutLoggedOutOAuth with Layout #2964

Merged
merged 3 commits into from
Feb 5, 2016

Conversation

drewblaisdell
Copy link
Contributor

Fixes #2960. The layout changes based on the current section and presence of a user prop.

LayoutLoggedOutDesign hasn't been removed yet because it is also rendered on the server and has dependencies (like abtest) that assume a browser environment. I started working on this, but updating Layout to run on the server is a rather large task, and will be handled separately - I'll open an issue for this once this is merged.

Testing
There are no visual changes. We need to assert that the layout doesn't break when the user is logged out, logged in, using the desktop app, and on /design.

Logged out

  • Visit /start while logged out, and assert that the page isn't broken.
  • Go through the signup flow an ensure the email verification notice shows.

Logged in

  • Visit any page while logged in, and assert that the page isn't broken.

Using the desktop app

  • Check out this branch inside the calypso/ directory in the desktop app (cd calypso && git checkout update/one-layout).
  • make run from wp-desktop/ and assert that the login page isn't broken.
  • Log in and assert that the Reader (or any other page, once logged in) isn't broken.

/design

  • Visit /design while logged out and assert that the page isn't broken.
  • Code review
  • Product review

@drewblaisdell drewblaisdell self-assigned this Feb 1, 2016
@drewblaisdell drewblaisdell force-pushed the update/one-layout branch 2 times, most recently from 9b8cb63 to d60439b Compare February 2, 2016 20:45
@drewblaisdell drewblaisdell changed the title Layout: Render Layout on all routes Layout: Replace LayoutLoggedOut and LayoutLoggedOutOAuth with Layout Feb 2, 2016
}
.is-section-login {
background: $blue-wordpress;
height: 100%;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block obviates most of these styles. They should be removed once this is merged.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block obviates most of these styles. They should be removed once this is merged.

I created an issue for that here: https://github.com/Automattic/wp-desktop/issues/67

@drewblaisdell drewblaisdell added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Feb 2, 2016
@@ -81,7 +82,23 @@ Layout = React.createClass( {
return sortBy( this.props.sites.get(), property( 'ID' ) ).pop();
},

renderMasterbar() {
renderEmailVerificationNotice: function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be called anywhere anymore.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it in eb1059e22ce0c16682238e8ea22ab282541297fc

@scruffian
Copy link
Member

Code looks good. @drewblaisdell I added a couple of tiny commits.

<Welcome isVisible={ showWelcome } closeAction={ this.closeWelcome } additionalClassName="NuxWelcome">
<WelcomeMessage welcomeSite={ newestSite } />
</Welcome>
<TranslatorInvitation isVisible={ showInvitation } />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

showInvitation is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks for catching that, @stephanethomas.

@stephanethomas stephanethomas added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 4, 2016
@drewblaisdell drewblaisdell added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 4, 2016
@scruffian
Copy link
Member

Tested again on desktop, logged in and logged out. LGTM.

@scruffian scruffian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 5, 2016
scruffian added a commit that referenced this pull request Feb 5, 2016
Layout: Replace `LayoutLoggedOut` and `LayoutLoggedOutOAuth` with `Layout`
@scruffian scruffian merged commit 4e716a5 into master Feb 5, 2016
@scruffian scruffian deleted the update/one-layout branch February 5, 2016 16:15
@scruffian
Copy link
Member

LayoutLoggedOutDesign hasn't been removed yet because it is also rendered on the server and has dependencies (like abtest) that assume a browser environment. I started working on this, but updating Layout to run on the server is a rather large task, and will be handled separately - I'll open an issue for this once this is merged.

I did this in #3112.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants