Framework: Add `makeLayout` #5081

ockham · 2016-04-28T12:40:49Z

This PR:

Has Layout accept React element trees as primary, secondary, and tertiary props.
Splits client/controller into a client and server side version.
Changes makeLayout() to automatically render LayoutLoggedOut in logged-out mode, and Layout otherwise.
Also adds ReduxWrappedLayout and ReduxWrappedLoggedOutLayout helper components.
Has my-sites/controller expose makeNavigation as a middleware that populates context.secondary
Changes client/my-sites/themes to use makeLayout

No visual changes.

To test:

Make sure the theme showcase still works as before.
Specifically, test landing on (logged-in) /design, and clicking on a theme's screenshot to navigate to its details sheet.

Fixes part of #2299. This is a redux of #4836, also picking up some pieces from #4820.

seear · 2016-04-29T10:39:31Z

Testcase:

http://calypso.localhost:3000/design
Pick a theme and select Choose this design
hit browser back button
search for something with no results
Actual:
signup content is still on the page:

This is not happening on master.

ockham · 2016-04-29T11:25:22Z

Ugh, good spot. Another point where React is apparently confused by multiple subtrees (signup's and the showcase's) rendering to #primary, mainly because of makeLayout and single-tree Layout. While adding keys to Layout seems to fix this, it introduces other errors when switching between 'My Sites' and 'Reader' while logged in. Hence, I'm going to branch off the MVP (726dfa9f2a68449940cdbe354bdd7b5285e14968, plus some minor cleanup commits) for now.

mtias · 2016-04-29T12:52:16Z

client/controller/index.node.js

+			<LayoutComponent store={ store }
+				primary={ primary }
+				secondary={ secondary }
+				tertiary={ tertiary }


We are not really using tertiary anymore, we should drop all references to it.

I still see it being used in https://github.com/Automattic/wp-calypso/blob/master/client/reader/controller.js...

Ugh, that should use root-child.

Filed #5182.

mtias · 2016-04-29T12:56:58Z

Any ideas on how this would affect chunk loading and file size?

ockham · 2016-04-29T17:01:53Z

Any ideas on how this would affect chunk loading and file size?

AFAICT, the main change is that sections -- via bundler/loader -- now uses makeLayout, and hence (logged-in) Layout, along with the latter's dependencies. Doesn't seem to bad:

master:

                                        Asset     Size  Chunks             Chunk Names
                media.84c2f554fe3be4b69e30.js   289 kB      15  [emitted]  media
              commons.4a5397d00eb92032dc17.js   303 kB       0  [emitted]  
          posts-pages.0a8866c6ecd41e7d3d3e.js   439 kB       2  [emitted]  posts-pages
               signup.d14f47389c40923355be.js   623 kB       3  [emitted]  signup
          post-editor.ebc0709c47c57b974b09.js  3.48 MB       4  [emitted]  post-editor
              devdocs.98219078be754d441c50.js  1.29 MB       5  [emitted]  devdocs
                theme.f471e68ffe35c59fba75.js   307 kB       6  [emitted]  theme
             upgrades.ad40e91eaaaa163c311a.js  1.04 MB       7  [emitted]  upgrades
              plugins.aebef1c9bb6e51145231.js   641 kB       8  [emitted]  plugins
                stats.690a5ded34871d0146e2.js   546 kB       9  [emitted]  stats
             settings.1ca434e40644411281fa.js   491 kB      10  [emitted]  settings
                menus.dcc17ba800cdeba77031.js   372 kB      11  [emitted]  menus
               people.c5e9f034483228237a8e.js   316 kB      12  [emitted]  people
             security.30bf6b90d127da2a2577.js   307 kB      13  [emitted]  security
                plans.56310a5466715e634c31.js   289 kB      14  [emitted]  plans
               reader.9a46f00f8e5963d947da.js  1.08 MB       1  [emitted]  reader
              sharing.668c4c15589a0e2212fe.js   316 kB      16  [emitted]  sharing
            purchases.c26c992c7add99c8357e.js   263 kB      17  [emitted]  purchases
notification-settings.c1f0439bb1c7ce4b3491.js   138 kB      18  [emitted]  notification-settings
        accept-invite.26dccb700c6beaa3f9b5.js   176 kB      19  [emitted]  accept-invite
                 help.6ee6a41e7875f7665265.js   102 kB      20  [emitted]  help
              billing.241fe83bddb894dde74d.js  73.2 kB      21  [emitted]  billing
              account.1adcae4da1c59f54b979.js  74.7 kB      22  [emitted]  account
            customize.5b98a332567fcbd4d49c.js  52.3 kB      23  [emitted]  customize
                  ads.81da350ee1adb3ff1bc2.js  69.2 kB      24  [emitted]  ads
         posts-custom.5a384eb685fb32639c49.js  39.7 kB      25  [emitted]  posts-custom
        mailing-lists.25b6042fcf3c170ef084.js    17 kB      26  [emitted]  mailing-lists
                   me.f5320dd5774be2b2d8c4.js  3.32 kB      27  [emitted]  me
               vendor.4a5397d00eb92032dc17.js  1.04 MB      28  [emitted]  vendor
           devmodules.17a8240f3f0d6ead1503.js  9.91 kB      29  [emitted]  devmodules
    build-development.4a5397d00eb92032dc17.js  3.85 MB      30  [emitted]  build-development

This branch:

                                        Asset     Size  Chunks             Chunk Names
                media.3d94bc48df41067b6c02.js   289 kB      15  [emitted]  media
              commons.64fbdc5e97c81bd9baf7.js   306 kB       0  [emitted]  
          posts-pages.ff902c4dc22c2e794078.js   439 kB       2  [emitted]  posts-pages
               signup.7428528cca73a1b2ea5e.js   612 kB       3  [emitted]  signup
          post-editor.b2cc47346dbb98bf1425.js  3.48 MB       4  [emitted]  post-editor
              devdocs.dd1acfdc559dfdf15f1d.js  1.29 MB       5  [emitted]  devdocs
                theme.1e53961f159d4380a628.js   307 kB       6  [emitted]  theme
             upgrades.8ba4f2cd2348a190432d.js  1.04 MB       7  [emitted]  upgrades
              plugins.cd9e7cf21d38f8a5767f.js   641 kB       8  [emitted]  plugins
                stats.503d23ea6d455b153b77.js   546 kB       9  [emitted]  stats
             settings.63bf3b8ec5e787d64df3.js   491 kB      10  [emitted]  settings
                menus.1ddc0067311e84d131e1.js   372 kB      11  [emitted]  menus
               people.064aec32c1e96c16daa2.js   316 kB      12  [emitted]  people
             security.d2226c765baa89e48ae9.js   307 kB      13  [emitted]  security
                plans.c61958eefa3948ade472.js   289 kB      14  [emitted]  plans
               reader.8f0ed84aef0f2cc433fb.js  1.08 MB       1  [emitted]  reader
              sharing.1825bd8c33b8939f274e.js   316 kB      16  [emitted]  sharing
            purchases.df05bc6a469fdbea2db1.js   263 kB      17  [emitted]  purchases
notification-settings.b410f654deda84ebc9b9.js   138 kB      18  [emitted]  notification-settings
        accept-invite.d50aa21d8e3a14c4025b.js   176 kB      19  [emitted]  accept-invite
                 help.fc5ef805b0566048b19a.js   102 kB      20  [emitted]  help
              billing.13bbc2bb34f30a0c5057.js  73.2 kB      21  [emitted]  billing
              account.6a08a9838deeac8fd435.js  74.7 kB      22  [emitted]  account
            customize.ddf9919ab0e572b704d9.js  52.3 kB      23  [emitted]  customize
                  ads.749f79c66918e46ff9d4.js  69.2 kB      24  [emitted]  ads
         posts-custom.efb26192053d0e050364.js  39.7 kB      25  [emitted]  posts-custom
        mailing-lists.5fad2169b4a058e3f651.js    17 kB      26  [emitted]  mailing-lists
                   me.1f9c7ac49cb75f5e23e5.js  3.32 kB      27  [emitted]  me
               vendor.64fbdc5e97c81bd9baf7.js  1.04 MB      28  [emitted]  vendor
           devmodules.6f67b538eaffd713ac81.js  9.91 kB      29  [emitted]  devmodules
    build-development.64fbdc5e97c81bd9baf7.js  3.85 MB      30  [emitted]  build-development

ockham · 2016-04-29T17:05:16Z

Subtree unmounting and reconciliation is quite a headache here, and to fix errors like #5081 (comment) we might have to force a re-render of the entire Layout at some more points than we're currently doing.

Some preliminary experiments of mine seem to indicate that React 15 is more reliable wrt to that kind of issue, so it might make sense to upgrade to that first (#5116) before spending more time tinkering with re-renders because of those pesky errors...

ockham · 2016-08-08T22:07:05Z

That previous error reported by @seear should've been fixed by 08c2e60.

ehg · 2016-08-09T17:03:37Z

This is very cool. I've been trying to break it by trying different routes, entry points, exit points, and logged in states. All work.

The code looks good from an initial glance, but I'll have a more thorough look later.

Could you rebase this against master now that the node upgrade has been merged? nvm use && make clean distclean && make run is getting tedious :]

ockham · 2016-08-09T18:19:47Z

This is very cool. I've been trying to break it by trying different routes, entry points, exit points, and logged in states. All work.

Yay!

Could you rebase this against master

Yep, done.

* Allow elements as props to the layout component * Add a ReduxWrappedLayout helper function * Split the entry points for the isomorphic router controller (allowing client-specific dependencies)

ehg · 2016-08-09T20:41:28Z

client/controller/index.web.js

 import LayoutLoggedOut from 'layout/logged-out';
+import nuxWelcome from 'layout/nux-welcome';
+import translatorInvitation from 'layout/community-translator/invitation-utils';
+import { makeLayoutMiddleware } from './index.node.js';


This should go in shared.js right? :)

ehg · 2016-08-09T20:52:02Z

Had a closer look at the code. This looks good to 🚢 . One 🌲 to rule them all!

SSR was broken by #5081, where I flipped the check for logged-out to logged-in: https://github.com/Automattic/wp-calypso/pull/5081/files#diff-2ff5c9f67cfab8b6325e63706e3f4970R18 misses the negation that was previously present at https://github.com/Automattic/wp-calypso/pull/5081/files#diff-a3600f12cca835b2ea80190a8243a8cbL26. This PR adds it back.

ockham self-assigned this Apr 28, 2016

ockham added [Feature Group] Appearance & Themes Features related to the appearance of sites. Framework [Status] In Progress labels Apr 28, 2016

ockham added this to the Themes: Showcase M4-ThemeSheets milestone Apr 28, 2016

ockham force-pushed the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch 3 times, most recently from 805277e to cff921e Compare April 28, 2016 20:19

ockham 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 Apr 28, 2016

ockham force-pushed the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch from 84e8ef5 to aa3861b Compare April 29, 2016 12:03

ockham changed the title ~~Framework: Add makeLayout, don't render layout for isomorphic sections in client/boot~~ Framework: Add makeLayout Apr 29, 2016

mtias reviewed Apr 29, 2016
View reviewed changes

ockham mentioned this pull request May 3, 2016

Reader: Use root-child instead of tertiary, drop the latter #5182

Closed

ockham force-pushed the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch 3 times, most recently from a6413d8 to 058a092 Compare May 3, 2016 22:49

ockham mentioned this pull request May 24, 2016

Framework: <App> component #378

Closed

ockham added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 24, 2016

folletto modified the milestones: Themes: Future, Themes: Showcase M4-ThemeSheets Jun 22, 2016

ockham mentioned this pull request Jun 27, 2016

Framework: Add support for logged in isomorphic layout rendering #6312

Closed

ockham force-pushed the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch from 08c2e60 to b5c21b3 Compare August 9, 2016 18:19

seear and others added 12 commits August 9, 2016 22:00

Framework: Add makeLayout() middleware

db728f2

* Allow elements as props to the layout component * Add a ReduxWrappedLayout helper function * Split the entry points for the isomorphic router controller (allowing client-specific dependencies)

Framework: fix layout section proptype

e779050

Themes: Remove obsolete comment

f448723

my-sites/controller: Export makeNavigation middleware

33ecc30

Themes: Use makeNavigation and makeLayout for rendering logged-in

2b0e7fc

client/boot: Remove an obsolete check

201a080

my-sites/controller: Use JSX for createNavigation() return value

bda0b78

client/controller: Make README.md more verbose

29571a7

client/controller: Move shared code to shared.js

97fce7c

Framework: Unmount multi-tree layout before rendering single-tree

4989186

Themes: Remove siteSelection middleware from multi-site route

69fb078

Layout: Conditionally add wp-singletree-layout class

f892793

ockham force-pushed the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch from b5c21b3 to f892793 Compare August 9, 2016 20:03

client/controller: Drop layoutFocus import

30d5ad7

ehg reviewed Aug 9, 2016
View reviewed changes

client/controller: Move makeLayoutMiddleware to shared.js

cbcb62b

ehg 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 Aug 9, 2016

ockham merged commit 0982dd4 into master Aug 10, 2016

ockham deleted the update/client-boot-dont-render-layout-for-isomorphic-sections-take-two branch August 10, 2016 12:09

ockham mentioned this pull request Aug 22, 2016

Server Side Rendering: Fix #7597

Merged

lancewillett removed the [Status] Ready to Merge label Oct 19, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Add `makeLayout` #5081

Framework: Add `makeLayout` #5081

ockham commented Apr 28, 2016 •

edited

Loading

seear commented Apr 29, 2016

ockham commented Apr 29, 2016

mtias Apr 29, 2016

ockham Apr 29, 2016

mtias Apr 29, 2016

ockham May 3, 2016

mtias commented Apr 29, 2016

ockham commented Apr 29, 2016

ockham commented Apr 29, 2016

ockham commented Aug 8, 2016

ehg commented Aug 9, 2016 •

edited

Loading

ockham commented Aug 9, 2016

ehg Aug 9, 2016

ehg commented Aug 9, 2016

Framework: Add makeLayout #5081

Framework: Add makeLayout #5081

Conversation

ockham commented Apr 28, 2016 • edited Loading

seear commented Apr 29, 2016

ockham commented Apr 29, 2016

mtias Apr 29, 2016

Choose a reason for hiding this comment

ockham Apr 29, 2016

Choose a reason for hiding this comment

mtias Apr 29, 2016

Choose a reason for hiding this comment

ockham May 3, 2016

Choose a reason for hiding this comment

mtias commented Apr 29, 2016

ockham commented Apr 29, 2016

ockham commented Apr 29, 2016

ockham commented Aug 8, 2016

ehg commented Aug 9, 2016 • edited Loading

ockham commented Aug 9, 2016

ehg Aug 9, 2016

Choose a reason for hiding this comment

ehg commented Aug 9, 2016

Framework: Add `makeLayout` #5081

Framework: Add `makeLayout` #5081

ockham commented Apr 28, 2016 •

edited

Loading

ehg commented Aug 9, 2016 •

edited

Loading