Layout: Add `preview` layoutFocus for `DesignPreview` #4690

sirbrillig · 2016-04-12T22:17:10Z

Summary

Adds a new layoutFocus called preview which, when activated (layoutFocus.set( 'preview' )), shows a preview of the current site pretty much anywhere in Calypso.

How it works

The DesignPreview component can render raw markup to a preview and then apply customizations to that preview. This PR adds a data layer for the preview markup and the customizations.

The raw markup is fetched from the REST API and saved as previewMarkup in the Redux state tree (one for each site). When it is rendered, the method updatePreviewWithChanges() in lib/design-preview will be called with the arguments:

The document object of the iframe.
The current value of the customizations object from the Redux state tree for this site.

The customizations object and the previewMarkup used by the preview are stored in the Redux state tree under the preview key for a given site. The state also keeps track of the "saved" status of the customizations and a history of all customization changes (enabling an "undo" feature).

This PR is essentially a newer version of several parts of #4140 and #4153 except without DesignMenu, the updaters, or the save-functions, which can be added separately.

Testing

To activate it right now you can press the "customize" button in the sidebar, but that is only temporary and should be removed before merging. It would be nice if we could replace that with a command-line method of activating the layout. Not sure how to go about that...

sirbrillig · 2016-04-12T22:19:11Z

Ping @mtias, @ehg, and @mcsf for comments

mcsf · 2016-04-22T12:08:44Z

client/lib/wpcom-undocumented/lib/undocumented.js

+		} )
+		.catch( reject );
+	} );
+};


Two notes:

I think we should add one step of indentation when starting a line with .then, .catch, or any other property/method access, and that is AFAIK the convention in Calypso.

I'd refactor things here, as the handling of promises is repeated verbatim in the function:

const request = postData && Object.keys( postData ).length > 0 ? this.wpcom.req.post( endpoint, query, { customized: postData } ) : this.wpcom.req.get( endpoint, query ); request .then( response => { … } ) .catch( reject );

As a side effect, it makes the function more self-explanatory.

Good points! I don't like long ternaries as I think they decrease readability, but I also think combining the requests is a good idea, so I compromised and made a shorter ternary with some extra consts.

mcsf · 2016-04-27T12:16:44Z

client/lib/wpcom-undocumented/lib/undocumented.js

+		const endpoint = `/sites/${siteId}/previews/mine`;
+		const query = { path: slug };
+		const isPreviewCustomized = ( postData && Object.keys( postData ).length > 0 );
+		const { post, get } = this.wpcom.req;


Interesting. I didn't expect post and get to be automatically bound to req.

Oh yeah, interesting indeed. I didn't expect that get or post would use this, but there it is. It looks like maybe binding to the right-hand-side object is part of object destructuring assignment.

It still makes me uncomfortable, though. :) And, actually, I'm not convinced that there's any magic binding happening: see Babel output.

I'd keep the method calls attached to the whole context tree (this.wpcom.req.post( bla )). Then again, I am not bothered by three-line ternaries. ;)

Fair enough. Changed to the longer version.

lsinger · 2016-04-29T12:05:40Z

This looks great! Two things I noticed after playing around a bit:

When I change my site (in this case, I changed my header image) the preview doesn't refresh, even if I reload Calypso.dev. Should it notice changes by itself? Should I have a way to refresh?
When I open the preview, it opens in tablet size by default. I prefer desktop size, so I set it to that. I close the preview. I open the preview again. It's now in desktop size, great--but apparently the CSS stops working. Can anyone reproduce or is it just me?

sirbrillig · 2016-05-02T20:12:54Z

When I change my site (in this case, I changed my header image) the preview doesn't refresh, even if I reload Calypso.dev. Should it notice changes by itself? Should I have a way to refresh?

I think that was a bug having to do with the site ID not being available when the component loads. I've just fixed it in 9153d4e. Let me know if that solves the issue for you.

When I open the preview, it opens in tablet size by default. I prefer desktop size, so I set it to that. I close the preview. I open the preview again. It's now in desktop size, great--but apparently the CSS stops working. Can anyone reproduce or is it just me?

@lsinger Interesting! I wasn't able to reproduce but it may be something about your particular site. Can you share the site with me? (we can do this via Slack.)

sirbrillig · 2016-05-02T20:13:15Z

Rebased to remove conflicts. Sorry to anyone who had this checked out.

lsinger · 2016-05-03T10:06:17Z

I think that was a bug having to do with the site ID not being available when the component loads. I've just fixed it in 9153d4e. Let me know if that solves the issue for you.

Yup, reloading Calypso does the trick now. 👍

Would it make sense to (while the preview is open) regularly poll the API whether anything about the site has changed?

Interesting! I wasn't able to reproduce but it may be something about your particular site. Can you share the site with me? (we can do this via Slack.)

Sent it to you in Slack. See also the following font differences.

Actual site:

Preview:

sirbrillig · 2016-05-03T22:30:35Z

Would it make sense to (while the preview is open) regularly poll the API whether anything about the site has changed?

Hm... Probably? OTOH polling is expensive, especially here because the preview is large. I'm open to discussion but I think it might be best to put that off as a feature for later (perhaps with a more efficient mechanism to query for a changed site).

lsinger · 2016-05-04T07:56:12Z

it might be best to put that off as a feature for later

That's what I thought, just wanted to make sure I'd mentioned it. :)

ehg · 2016-05-05T13:55:12Z

client/state/preview/reducer.js

+/**
+ * External dependencies
+ */
+import assign from 'lodash/assign';


Any reason not to use Object.assign instead of lodash?

Because, as I found out relatively recently, Object.assign is ES6 and (at least last time I checked) we don't support polyfills for prototype methods in Calypso. So if I use Object.assign() then it will only work in browsers that support that feature.

It isn't a prototype method :) Ref: #2360 (comment)

Half true. We're not actually using babel/polyfill, we're using the babel/runtime with webpack, which only polyfills non-prototype things. So we get Map and Set and Object.assign, but not things on String.prototype like startsWith.

Ohhhhh... TIL. Thanks!

ehg · 2016-05-05T15:15:00Z

I think this is looking like a pretty solid start. Nice job. No doubt we'll find bugs/missing features when we use it in anger, but for me it makes sense to remove the 'Customize' hijack and merge this now.

@lsinger @mcsf what do you think?

ehg · 2016-05-05T15:19:09Z

client/my-sites/design-preview/index.js

+		// Refresh the preview when it is being shown (since this component is
+		// always present but not always visible, this is similar to loading the
+		// preview when mounting).
+		if ( this.props.showPreview && ! prevProps.showPreview ) {


Nifty interim solution!

lsinger · 2016-05-06T12:45:10Z

I think this is looking like a pretty solid start. Nice job. No doubt we'll find bugs/missing features when we use it in anger, but for me it makes sense to remove the 'Customize' hijack and merge this now.

The incorrect fonts still worry me--also could point to a deeper problem.

ehg · 2016-05-06T13:08:00Z

The incorrect fonts still worry me--also could point to a deeper problem.

Hmm, interestingly the webfonts work for me in Firefox, but not in Chrome (consistently — I saw a brief flash). Taking a deeper look.

ehg · 2016-05-06T14:24:07Z

The incorrect fonts still worry me--also could point to a deeper problem.

Updating https://github.com/typekit/webfontloader on my WPCOM sandbox to the current version fixes this for Google Fonts in Chrome. TypeKit fonts will only work on wordpress.com domains.

ehg · 2016-05-06T14:29:16Z

I just got a 'your stats are booming!' notification. We shouldn't be bumping stats for previews…

sirbrillig · 2016-05-06T15:19:04Z

I just got a 'your stats are booming!' notification. We shouldn't be bumping stats for previews

Ah, yes. 😩 I had forgotten about this. It's an issue with the Previews endpoint. @mattwiebe might have some insight.

ehg · 2016-05-09T15:08:27Z

Seeing as the webfonts and stat bumping are issues independent of this PR, I think we should merge this, and track them elsewhere.

sirbrillig · 2016-05-09T16:07:19Z

I'll rebase to remove the sidebar hack and mark as Ready for Review

sirbrillig · 2016-05-09T17:01:51Z

Rebased. Sidebar "customize" hook is still available in 672cc9a4c10be24be8f90fee773dbee222596177 if needed for testing.

ehg · 2016-05-12T11:26:53Z

LGTM. and onwards to #5117 :)

aduth · 2016-05-20T13:50:24Z

client/my-sites/design-preview/index.js

+	loadPreview() {
+		if ( this.props.selectedSiteId && this.props.actions.fetchPreviewMarkup ) {
+			debug( 'loading preview with customizations', this.props.customizations );
+			this.props.actions.fetchPreviewMarkup( this.props.selectedSiteId, '', this.props.customizations );


Do we want to be triggering a network request for preview whenever navigating My Sites? At least, this is what I'm observing currently (assuming component is always mounted in the background?)

Hm. That doesn't sound right. I think my intent was that the preview request only load when either:

The my-sites area for a specific site first loads.

The preview is made visible (in case there are changes).

When customizations had been applied but have since been removed.

@sirbrillig Unfortunately that doesn't appear to be the case. Here's an example refreshing the settings page, showing the network request on page load:

Yeah, no good. We may want to speed up the merge of parts of #5117 which disable the preview endpoint.

The my-sites area for a specific site first loads.

The preview is made visible (in case there are changes).

When customizations had been applied but have since been removed.

an example refreshing the settings page, showing the network request on page load…

Doesn't this conform to the expected behaviour? I believe we want to load the preview on the initial page load, and for every site switched. We could switch it to fetch on demand, but we'd lose the 'instant' factor, which the editor preview has.

Doesn't this conform to the expected behaviour? I believe we want to load the preview on the initial page load, and for every site switched. We could switch it to fetch on demand, but we'd lose the 'instant' factor, which the editor preview has.

Eh, that feels a bit heavy. The likelihood that my session using My Sites will lead to previewing customizations is much much lower than the likelihood I'd preview a post I'm editing.

Not sure what the behavior is here, but we're also not doing background loading for mobile devices in the editor.

I'm not a fan of the editor background loading FWIW 😄 I wonder if there's a middle ground where we interpret "intent" (e.g. hover action).

I'd like to get @mtias input also because the background loading (once for each site, when it first loads) was originally his idea.

sirbrillig self-assigned this Apr 12, 2016

sirbrillig added the [Status] In Progress label Apr 12, 2016

sirbrillig force-pushed the add/preview-layout-focus branch 2 times, most recently from 2e203b7 to f00754c Compare April 21, 2016 16:05

mcsf reviewed Apr 22, 2016
View reviewed changes

mtias added Framework Navigation labels Apr 22, 2016

mcsf reviewed Apr 27, 2016
View reviewed changes

This was referenced Apr 29, 2016

Sidebar: use the "preview" layoutFocus to preview the current site #5117

Merged

GuidedTours: add a site preview step #5120

Closed

sirbrillig force-pushed the add/preview-layout-focus branch from e534473 to 9153d4e Compare May 2, 2016 20:11

ehg reviewed May 5, 2016
View reviewed changes

sirbrillig force-pushed the add/preview-layout-focus branch 2 times, most recently from 672cc9a to 777f003 Compare May 9, 2016 17:00

sirbrillig 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 May 9, 2016

sirbrillig added 9 commits May 11, 2016 14:43

WPCOM: add fetchPreviewMarkup to wpcom-undocumented

ef5f0c6

State: add preview redux tree and actions

df9a72f

DesignPreview: add DesignPreview wrapper to WebPreview

2b0bb13

Layout: move connect to correct import block

6b38870

Layout: add DesignPreview to layout if 'preview-layout' is set

cbe9bf6

Config: add preview-layout config option to development

e966c9e

LayoutFocus: add preview layout

5c93916

Layout: add styles for DesignPreview

a31b574

WebPreview: improve debug messages for markup

588d24e

ehg force-pushed the add/preview-layout-focus branch from 777f003 to 588d24e Compare May 11, 2016 14:04

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 May 12, 2016

sirbrillig merged commit 44db302 into master May 12, 2016

sirbrillig deleted the add/preview-layout-focus branch May 12, 2016 16:10

matticbot removed the [Status] Ready to Merge label May 12, 2016

aduth reviewed May 20, 2016
View reviewed changes

aduth mentioned this pull request Jan 18, 2017

State: Ensure that all unhandled actions return same state #10761

Merged

1 task

Layout: Add preview layoutFocus for DesignPreview #4690

Layout: Add preview layoutFocus for DesignPreview #4690

Conversation

sirbrillig commented Apr 12, 2016 • edited Loading

Summary

How it works

Testing

sirbrillig commented Apr 12, 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

Choose a reason for hiding this comment

sirbrillig May 2, 2016 • edited Loading

Choose a reason for hiding this comment

lsinger commented Apr 29, 2016

sirbrillig commented May 2, 2016

sirbrillig commented May 2, 2016

lsinger commented May 3, 2016

sirbrillig commented May 3, 2016

lsinger commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirbrillig May 5, 2016 • edited Loading

Choose a reason for hiding this comment

ehg commented May 5, 2016 • edited Loading

ehg May 5, 2016 • edited Loading

Choose a reason for hiding this comment

lsinger commented May 6, 2016

ehg commented May 6, 2016

ehg commented May 6, 2016

ehg commented May 6, 2016

sirbrillig commented May 6, 2016

ehg commented May 9, 2016

sirbrillig commented May 9, 2016

sirbrillig commented May 9, 2016

ehg commented May 12, 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

ehg May 20, 2016 • edited Loading

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

Layout: Add `preview` layoutFocus for `DesignPreview` #4690

Layout: Add `preview` layoutFocus for `DesignPreview` #4690

sirbrillig commented Apr 12, 2016 •

edited

Loading

sirbrillig May 2, 2016 •

edited

Loading

sirbrillig May 5, 2016 •

edited

Loading

ehg commented May 5, 2016 •

edited

Loading

ehg May 5, 2016 •

edited

Loading

ehg May 20, 2016 •

edited

Loading