[UX] Bring back node preview

[docwilmot]

We removed node edit previews early on, but haven't agreed on how to re-introduce it. I think this is expected functionality.
Lets decide how to bring it back.

---

PR: backdrop/backdrop#2174
PR: backdrop/backdrop#2290

[ghost]

I like how GitHub has a preview tab that doesn't require saving first and is easy to switch back and forth. Is something like that possible?

[findlabnet]

This is normal when content of node edited under admin theme, so we have another context of styling. What we expect to show in this preview?

[olafgrabienski]

What we expect to show in this preview?

I'd expect a responsive preview of the whole page (not only the edited content) in the look and feel of the active frontend theme. Maybe the preview page can be displayed in an iFrame.

Recently I saw an interesting Drupal responsive preview module but haven't tested it so far: https://www.drupal.org/project/responsive_preview

[docwilmot]

@quicksketch @jenlampton was looking at the initial discussion about how D8 got live previews https://www.drupal.org/project/drupal/issues/1510544, where you both commented. We have tempstore in core so this is doable here. We dont have entity UUIDs though so our solution may be a bit more volatile. I was considering:

• on node preview assign a tempstore ID based on uid and node type
• if the user navigates away then when he attempts to create a new node of that type load from tempstore and require either save or cancel before allowing a new node.

Would that work?

[docwilmot]

Did it, mostly copying Drupal.
Still very preliminary but nothing complex left to do I'd think.
The cancel button is broken with this.

[docwilmot]

@jenlampton here's a (cheeky) suggestion. The issue of poor previews in D8 was considered a critical bug. Can we consider no previews at all in Backdrop also a critical bug and fix it for 1.9?
(This PR isnt ready; I only pushed it because I seemed to have messed up my local repo somehow and it was past my bedtime. But the rest of the work is mostly cosmetic, and can finish this weekend.)

[jenlampton]

Nice try @docwilmot ;) We considered the inaccurate previews a critical bug in Backdrop too, which is why we removed them. The critical bug has been solved.

Now what we have is a feature request (and one that had been relegated to contrib first too, IIRC?). As soon as we have a solution we can slate it for the next minor release for core. I have a feeling something as important as previews will need to add/change enough about core that @quicksketch will want extensive testing, and it will be classified as a feature. But, Perhaps 1.11?

Did it, mostly copying Drupal.

Ooh! sounds like we maybe close? Jen goes and plays with new preview

[jenlampton]

My initial feedback is very positive! The preview works as expected, and in the correct front-end theme! It needs some UX cleanup, but it's a lot closer than I expected :) Nice work @docwilmot!!

I'm not sure how much my initial reaction has been colored by past drupal experience (probably a lot) but here's my initial feedback:

1. This is the doozy: What about layouts? If a node is to be rendered in a 3 column layout with a field in each column, is that taken into account in the preview? It needs to be, but I think that makes the display-mode switcher approach less viable... unless maybe we can use that to re-render the whole page? Maybe that's what it does already?

2. when I clicked the preview button, I was surprised that I left the page I was editing, and I was worried that all my changes had been lost, or were about to be lost if I did something wrong. We may want to add a message like we do for views & layouts letting people know that their changes have been saved temporarily.
   I was initially expecting either a modal preview, or something to appear on the same page, but after playing with this a little, I actually like it better, as what I'm looking at is almost what it will look like when it's published.

3. In order to make it clearer what on the preview page is real content, and what on the page is editorial tooling, we should style the .node-preview-container differently, somehow.

4. I wanted a "save" button directly on the preview page. If I'm happy with what I see I don't want to go back to the node form in order to save the node.

5. The language & location of the return to editing link was unclear to me. Maybe if it looked like an Edit button or tab that would be better?

6. the display-modes drop-down needs a label

7. we should remove the search and RSS display modes. This is a larger/separate problem See [UX] Add a 'visible' property to display modes #3082

8. The positioning of the preview button worried me initially. I'm not sure why - that's where it used to be... but I was looking for it at the top of the page for some reason? Not sure if this is valid feedback but leaving it here incase others experience it too.

edit: switched first and last items for order-of-importance

[docwilmot]

Didnt think you'd fall for that.
The doozy is something I didnt consider. Good point. Should be doable I think, will have a look.
The other significant point is the position of the .node-preview-container. It is in fact supposed to be at the top of the page. In D8 they have hook_page_build to position stuff on the page, but we dont. I was evaluating hacking Standard renderer to place it. I'll wait for other ideas from @quicksketch and anyone else.
Other things you list are/were in the plan, but since we're taking 4 more months to get this done, no rush.

I was initially expecting either a modal preview, or something to appear on the same page

Ughh, that's like so 2006! 😄

[olafgrabienski]

One small observation: I was previewing a post (let's say "Post One") and noticed its path node/post/preview (without a node ID). For curiosity I tried to add another post without canceling or saving the previewed post before. Instead of adding a new post I ended up in the form of "Post one", including the message Continue editing this post or cancel to clear.. Can we assign a (temporary) node ID to previews to avoid this conflict?

[docwilmot]

@olafgrabienski good point, and this was considered, as mentioned in #3062 (comment) above. For a new node, without yet a node ID, we could assign a random unique ID (the initial D8 patch just used time()).

But then if you navigated away from the preview, unless you remembered what the unique ID was (unlikely if is 9842946248 or BY87-87f7-f65f4-FRT8), you'd lose your work. So we'd need to add an administrative list of incomplete nodes, and/or a garbage collection to destroy old half-finished nodes on cron. We'd be treating these like revisions rather than just a preview, and this would be more complicated.

This would be a reasonable alternative, as it would make 'previews' more disposable - if you dont like what you see, just node/add and start again. But it is also more likely to cause alarm when someone thinks theyve lost their work if the unintentionally navigate away.

This PR probably doesnt help much with the initial alarm but at least a new user would quickly learn, if I navigate away, just go back to node/add and continue.

Advantages both ways, but less complexity this way I feel. Other opinions welcome though. @klonos?

Edit: to your actual point, the only way to get an nid so we get a node/nid path is to save the node, which is a whole lot more problems (hooks would fire, Rules would trigger, we'd need to re-write the admin list, etc, etc, etc.) Not an easy solution.

[olafgrabienski]

@docwilmot I see the advantage of the current PR and the hurdles regarding the node ID.

Actually, I was mainly concerned about conflicts between different editors: What would happen if Editor A adds "Page one" and previews it while in the meantime Editor B adds another page. I guessed that Editor B would also end up in the "Page one" form of Editor A. But I've tested it now in the sandbox site, and everything seems fine: Editor B is able to add a page in its own form. (How does that work, btw?)

[docwilmot]

Actually, I was mainly concerned about conflicts between different editors

Understood.
The PR adds an entry in tempstore with an ID based on the UID and the Node type. So every user can have one in-preview node at any time.

[docwilmot]

This is the doozy: What about layouts? If a node is to be rendered in a 3 column layout with a field in each column, is that taken into account in the preview?

@jenlampton @quicksketch the only way (I can think of) to get a path to use a layout that is not explicitly defined for it (ie programmatically force a different layout) is, coincidentally, to use the new $renderer_name property we've added to the Layout class; for this example:

• manually create a layout config file for node/%/preview in the config active directory
• add a custom renderer_name and create the custom Renderer
• in that new renderer, override renderLayout() to check if the layout you want to use exists (check if a layout for node/% exists in this case) and use that layout instead.

This should work normally but I'm not sure if our preview URLs of node/NODE_ID/preview would actually resolve to node/%/preview int this case. But this isn't the point of this shout out to you.

I am actually wondering if special-casing node previews in Layout module would be simpler than all this, plus it really wouldnt do to have the preview layout in the layout list anyway.

Or if you had other ideas.

[quicksketch]

I haven't tried out the sandbox yet, it sounds rather exciting though. :)

Could we simply assign the node an ID when the preview is initiated? In WordPress, drafts are saved automatically as the content is created. I would think that a combination of auto-saving and then just showing the unpublished node page would achieve a nearly perfect preview. Essentially just streamline the normal approach of posting an unpublished node, viewing it, and then publishing it.

We currently have some problems in Backdrop that required fields must be entered before the form can be saved. Ideally we'd find a way to correct the dependence on the validation layer to tidy-up the data format, and make it so that content could auto-saved (or previewed) even before it was complete. Full validation of data on input would only be required when publishing the content.

[laryn]

@quicksketch FWIW the Save Draft module has an option to allow drafts to be saved even if required fields are not entered. Not sure if that is helpful here or not.

[docwilmot]

auto-saving

Oh wow, didn't think you'd lean that way after reading that long issue on Drupal. This would essentially be Save Draft in core as @laryn suspects, but with a "Preview" button instead.

Not sure the implications.

[olafgrabienski]

I would think that a combination of auto-saving and then just showing the unpublished node page would achieve a nearly perfect preview.

Personally, I often just visit the unpublished node page instead of using a preview, so I like this approach very much.

[docwilmot]

Had a look at how Wordpress does things. They have revisions in multiple states and the autosave saves in a "preview" state, not as live nodes. We dont have that, and we have no way to let modules know that our "preview" is not a save.

I personally dont like the idea of saving nodes before they are ready. Thinking of all the contrib modules which are set to do things when a node is created: messaging modules, ecommerce modules, Rules, and goodness knows what else all firing off when you prview a node. To start having users "saving" nodes without knowing that this is what theyre actually doing seems dangerous to me. This is something which should either follow an update to our revisions system (see https://www.drupal.org/project/drupal/issues/218755?) or in v2.0 when we can warn module builders of such a significant change.

My PR begins to provide essentially the same ability to view a node in close to the live state, and we can do it now.

[jenlampton]

My PR begins to provide essentially the same ability to view a node in close to the live state

It was the "close to" problem that got previews removed from core in the first place. We've got to figure out the layout bit in one way or other before this can get in. Maybe we can fake it in another way to generate a node context for layouts to use?

For the record, I'm also on the fence about adding auto-save before 2.0 as that does sound like a big change that could have serious implications.

[docwilmot]

Note I'm not even defending this PR or suggesting its the right way. I'm open to suggestions. But using the Tempstore approach is what D8 did, and it works well; it is essentially the same node viewed in the required view mode. Thats no where close to the madness of D7 previews.

Maybe we can fake it in another way to generate a node context for layouts to use?

This is actually what I was asking above about special-casing. We could circumvent all of the custom renderer_name shenanigans I mentioned by just tweaking the layout routing system itself, example:

function layout_get_layout_by_path($path = NULL, $router_item = NULL) {
  ...
  if (empty($cache[$router_item['path']])) {
    // If the path is a node preview path, check if a custom layout exists for
    // nodes and use that instead.
    if ($router_item['path'] == 'node/%/preview') { 
      $layouts = layout_load_multiple_by_path('node/%', TRUE);
    }
    else {
      $layouts = layout_load_multiple_by_path($router_item['path'], TRUE);
      ...

That would work.

[docwilmot]

Fixed so preview now uses a node layout if available.

[herbdool]

I like where it's going. It works well enough for most people I bet.

[docwilmot]

Thanks for having a look. This is a technical preview still. To be done:

• to place the preview bar at page top (not sure how yet)
• theming
• account for anonymous with permission to create content (could happen)
• account for alternative node layout path (like node/%/something/%)

But I think this, as you say, is a good enough approach, for now.

[docwilmot]

Thanks @herbdool.

Theres still this persistently failing date test that doesnt happen locally. I'm sure something like this has come up in the issue queue before, but I cannot remember where. (and I cant fix it if I cant replicate it). Annoying.

[quicksketch]

Theres still this persistently failing date test that doesnt happen locally.

Maybe try disabling Date module locally? The logic for handling date fields changes in the node form depending on whether it is enabled.

I propose a new hook_layout_get_layout_by_path_alter(&$layouts, $router_item, $object), invoked in layout module's layout_get_layout_by_path() function, that allows other modules to modify the layouts that are loaded for a given path before the chosen layout is resolved.

Let's continue talking about options for this in #1528, which is a closely related issue.

[quicksketch]

It looks like @docwilmot already created the new hook_layout_get_layout_by_path_alter() hook in the PR. I'd prefer we look at that problem separately in #1528. So for now I think leaving the hack with a @todo in place is better than rushing into an API addition that might cause us trouble.

And for whatever reason, that date failure seems to be caused by these changes. We've got lots of other PRs being updated that don't have these same failures.

[docwilmot]

Pushed again
Still 4 failing Date tests. Still all green locally with or without date. I can not figure it out,

[quicksketch]

I'll see if I can suss out the source of the failure. We can't commit this if it's breaking the tests.

[herbdool]

I see the exceptions locally when I test it too.

[quicksketch]

Looks like the source of the problem is that our date fields don't handle default values properly when the fields are hidden (not rendered). So for the sake of passing these tests, we just need to make the Created date field visible to the testing user by adding the "administer nodes" permission. We can make a follow-up for that new issue. I'll push a commit to the PR to fix up the tests.

[quicksketch]

I had some trouble rebasing the PR and somehow broke it... now it can't be reopened. I made a replacement PR at backdrop/backdrop#2290.

[herbdool]

Tested again and it looks good.

[quicksketch]

Great. As this has already been RTBC 3 times, with these issues addressed we're good to go. I think we will need to tidy-up the CSS further, but we already knew that.

I've merged backdrop/backdrop#2290 into 1.x for 1.11.0. Thanks everyone!