Themes: Add themes detail page #3053

ehg · 2016-02-03T20:45:12Z

Implements part of #2875 & #2876

This PR deals with setting up a /themes/[x] route, which will eventually contain details about the theme. We're only concerned with showing a fake, static title, real data comes later.

Notes:

Due to the way routing and layout currently works, i.e. two layout trees, not isomorphic, there are some hacks here to get SSR working properly. I've inline commented them.
isFullScreen layout prop introduced, so we can get rid of borders/margins/padding.

TODO:

Find a better name for isFullScreen
Reduce crazy shrinkwrap diff
Factor out the renderToString business

To test:

Try /design and click on a theme's menu, and choose details
Try logged in and logged out
Make sure navigation works as intended

Current UI state:

ehg · 2016-02-04T16:37:49Z

@mcsf — the routing bits here could inform #3065

mcsf · 2016-02-04T18:43:41Z

@mcsf — the routing bits here could inform #3065

👍

ockham · 2016-02-04T20:11:32Z

Possible that you didn't add my-sites/themes/sheet.jsx to this branch yet?

ockham · 2016-02-04T20:32:44Z

BTW, I just added a ThemeDetailsData fetcher component to #3011 :-)

ockham · 2016-02-05T14:59:00Z

Feature flag it?

seear · 2016-02-09T16:48:58Z

client/my-sites/themes/controller.js

+
+		console.log( 'running ze details controller' );
+
+		if ( true ) { // bail if we're logged out


Changing this to if ( user ) gets rid of the replacement error

ehg · 2016-02-09T17:39:15Z

Rebased against master.

Getting replaced root warnings when logged out and going from /themes/x to /design. Messy.

seear · 2016-02-09T18:23:24Z

Getting replaced root warnings when logged out and going from /themes/x to /design. Messy.

How are you doing this? Back button? Address bar? Both fine for me.

ehg · 2016-02-10T11:21:00Z

How are you doing this? Back button? Address bar? Both fine for me.

Direct hit on /themes/x, then pressing the back button.

ehg · 2016-02-10T16:44:19Z

client/boot/index.js

+			.filter( r => r.match !== null );
+
+
+		if ( matchedRoutes.length ) {


This is our own mini-router. We need to do this before the layout is rendered.

ehg · 2016-02-10T19:53:35Z

In the spirit of @mcsf's routing PR, I extracted the renderToString logic into its own module, and switched to using an unbounded lodash memoize for caching: 868de4e

ockham · 2016-02-15T00:05:04Z

I'd love if we combined the theme sheet with the routing from #3207.

Rationale -- I'd really like to avoid introducing new special cases that are spread over a couple of files, specifically

the mini-router in client/boot
making LayoutLoggedOutDesign even more particular to our routes, when I think it could be made quite generic relatively easy from its current state in master
the need to unmount secondary in the details route (this might be my most convincing argument)

I think we'll only need a couple of changes to server/pages on top of #3207 to make this work in a generic enough way (stealing some bits from this PR :] ) I'll try to come up with a PoC PR.

ockham · 2016-02-15T01:50:17Z

-> #3302

seear · 2016-02-15T14:58:11Z

I would rather see this PR merged sooner rather than later, since it is harder to build on top of while it is out on a branch. For example, it makes it hard to see the (pretty small) changes involved in #3279. Removing the ugly hacks later will be the payoff from the routing work.

seear · 2016-02-15T15:42:47Z

server/render/index.js

+ */
+import config from 'config';
+
+const memoizedRenderToString = memoize( ReactDomServer.renderToString, element => JSON.stringify( element ) );


Let's assume that any data retrieval has to happen outside of the renderToSting call, because there will always be some async component to it. Does that we are caching a call here that actually does very little, giving us an extra layer of cache alongside the data cache that may not save any time?

We don't have a data cache at the moment, though. I'd prefer some kind of caching that we could always remove later. Also, themes data isn't hugely variable, so I believe there will be some benefit to caching the markup — especially if we can share the markup between node instances.

ehg · 2016-02-15T15:57:42Z

I would rather see this PR merged sooner rather than later, since it is harder to build on top of while it is out on a branch.

OK, so should I split this into multiple PRs? e.g. render module, routing hacks & theme sheet component?

seear · 2016-02-15T16:20:22Z

client/boot/index.js

+
+		const themesRoutes = [
+			{ name: 'design', path: new Path( '/design' ) },
+			{ name: 'themes', path: new Path( '/themes/:theme_slug' ) },


Feature check here maybe?

seear · 2016-02-15T16:28:23Z

There are some layout problems for the rest of the site, even with the feature-flag turned off.

seear · 2016-02-15T16:54:18Z

Ok, looking good now. Ready to merge 👍

folletto · 2016-02-15T16:58:01Z

client/my-sites/themes/style.scss

+
+.themes__sheet-bar
+{
+	background-color: #0087be;


$blue-wordpress instead of hard coded color. :)

seear · 2016-02-15T17:02:05Z

(don't forget to squash...)

- add an isFullScreen option to layouts ( gets rid of side margins ) - unmount sidebar for sheet view - set no masterbar border for fullscreen mode

- implement a pre-layout router, that will be gone as soon as we have an isomorphic, single render tree layout solution. which is soon! - factor out renderToString into render module - use memoize, resolve by JSON.stringify( element ), use LruMap to make sure cache isn't unbounded

Themes: Add themes detail page

ockham · 2016-02-16T21:05:44Z

client/boot/index.js

+				props = { routeName: matchedRoutes[0].name, match: matchedRoutes[0].match };
+				Layout = require( 'layout/logged-out-design' );
+			}
+		} else if ( startsWith( '/design', window.location.pathname ) ) {


Isn't this the wrong arg order? cf L188 and https://lodash.com/docs#startsWith

Hah, yes. Why does it work?!

Cause this is only for logged-out, where we don't care for single sites. Then again, we should check if it breaks tiers.

Filed #3374 to fix this.

ehg added [Feature Group] Appearance & Themes Features related to the appearance of sites. [Status] In Progress labels Feb 3, 2016

ehg self-assigned this Feb 3, 2016

ehg added this to the Themes: Showcase M4-ThemeSheets milestone Feb 3, 2016

mcsf mentioned this pull request Feb 9, 2016

Themes: Isomorphic routing for /design [WIP/Experimental] #3065

Closed

5 tasks

seear reviewed Feb 9, 2016
View reviewed changes

ehg force-pushed the add/themes-sub-bar branch from c5e3345 to b619f61 Compare February 9, 2016 17:37

seear mentioned this pull request Feb 9, 2016

Framework: Deserialize bootstrapped state to allow conversion to immutable data #3187

Merged

ehg mentioned this pull request Feb 10, 2016

SSR: Only render into DOM if component tree is not already there #2939

Closed

ehg force-pushed the add/themes-sub-bar branch from 4376aa5 to 5c5b59d Compare February 10, 2016 16:40

ehg reviewed Feb 10, 2016
View reviewed changes

ehg force-pushed the add/themes-sub-bar branch 2 times, most recently from 625d8d0 to 868de4e Compare February 10, 2016 19:52

ehg 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 10, 2016

ehg force-pushed the add/themes-sub-bar branch from 868de4e to 0dad4f9 Compare February 10, 2016 20:14

seear force-pushed the add/themes-sub-bar branch from e588710 to 5fbd76f Compare February 12, 2016 14:12

seear mentioned this pull request Feb 12, 2016

Themes: Bootstrap theme-details store for theme sheets #3279

Merged

ockham mentioned this pull request Feb 15, 2016

Try/isomorphic routing #3302

Closed

ehg force-pushed the add/themes-sub-bar branch from 5fbd76f to 797cf2b Compare February 15, 2016 12:35

seear reviewed Feb 15, 2016
View reviewed changes

folletto reviewed Feb 15, 2016
View reviewed changes

ehg added 2 commits February 15, 2016 17:09

Themes: Initial theme sheet page

fa9ae2b

- add an isFullScreen option to layouts ( gets rid of side margins ) - unmount sidebar for sheet view - set no masterbar border for fullscreen mode

ehg force-pushed the add/themes-sub-bar branch from 77ba955 to 1f05fee Compare February 15, 2016 17:10

ehg added a commit that referenced this pull request Feb 15, 2016

Merge pull request #3053 from Automattic/add/themes-sub-bar

13f458b

Themes: Add themes detail page

ehg merged commit 13f458b into master Feb 15, 2016

ehg deleted the add/themes-sub-bar branch February 15, 2016 17:20

ehg removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 15, 2016

ehg mentioned this pull request Feb 15, 2016

Revert "Themes: Add themes detail page" #3311

Merged

ehg restored the add/themes-sub-bar branch February 15, 2016 17:25

ehg mentioned this pull request Feb 15, 2016

Themes: View a theme's title and author #2875

Closed

6 tasks

ockham reviewed Feb 16, 2016
View reviewed changes

ehg mentioned this pull request Feb 17, 2016

Themes: See a screenshot in the sheet view #2876

Closed

4 tasks

ehg deleted the add/themes-sub-bar branch February 24, 2016 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Themes: Add themes detail page #3053

Themes: Add themes detail page #3053

ehg commented Feb 3, 2016

ehg commented Feb 4, 2016

mcsf commented Feb 4, 2016

ockham commented Feb 4, 2016

ockham commented Feb 4, 2016

ockham commented Feb 5, 2016

seear Feb 9, 2016

ehg commented Feb 9, 2016

seear commented Feb 9, 2016

ehg commented Feb 10, 2016

ehg Feb 10, 2016

ehg commented Feb 10, 2016

ockham commented Feb 15, 2016

ockham commented Feb 15, 2016

seear commented Feb 15, 2016

seear Feb 15, 2016

ehg Feb 15, 2016

ehg commented Feb 15, 2016

seear Feb 15, 2016

seear commented Feb 15, 2016

seear commented Feb 15, 2016

folletto Feb 15, 2016

seear commented Feb 15, 2016

ockham Feb 16, 2016

ehg Feb 17, 2016

ockham Feb 17, 2016

ockham Feb 17, 2016


		console.log( 'running ze details controller' );

		if ( true ) { // bail if we're logged out

		.filter( r => r.match !== null );


		if ( matchedRoutes.length ) {

Themes: Add themes detail page #3053

Themes: Add themes detail page #3053

Conversation

ehg commented Feb 3, 2016

ehg commented Feb 4, 2016

mcsf commented Feb 4, 2016

ockham commented Feb 4, 2016

ockham commented Feb 4, 2016

ockham commented Feb 5, 2016

Choose a reason for hiding this comment

ehg commented Feb 9, 2016

seear commented Feb 9, 2016

ehg commented Feb 10, 2016

Choose a reason for hiding this comment

ehg commented Feb 10, 2016

ockham commented Feb 15, 2016

ockham commented Feb 15, 2016

seear commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehg commented Feb 15, 2016

Choose a reason for hiding this comment

seear commented Feb 15, 2016

seear commented Feb 15, 2016

Choose a reason for hiding this comment

seear commented Feb 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment