Problem/Motivation

Add responsive preview functionality.

Proposed resolution

Build solution on top or responsive_preview module.

But alter routes so that it uses CustomElementsController. Add support for the /layout-preview route in lupus_decoupled_layout_builder

Override PreviewUrl method so that it returns urls adjusted to a frontend.

Implements hook_toolbar() to add needed links where need be.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

useernamee created an issue. See original summary.

fago’s picture

This should add "layout-preview" route, but that would make more sense to add in the layout_builder sub-module

fago’s picture

Issue summary: View changes
fago’s picture

If possible, the integration with responsive-preview module should be optional, so it just works when the module is active, but it's not hard-dependency. The /layout-preview route can always be there when the module is active, I think that makes sense anyway.

roderik’s picture

Assigned: Unassigned » roderik

Trying to make this work on the base of preexisting Drunomics code. I'm sure the attribution will needs extending.

fago’s picture

let's organize code well and add a lupus_decoupled_responsive_preview sub-module for everything that requires responsive-preview.
we decided it's a good idea to do that for all contrib integrations

roderik’s picture

Draft notes:

Pushing current state of work: draft, as of two weeks ago. Only testing 'regular' preview so far. Maybe I will have an update later today, or maybe not for another two weeks again -- so I should push this now.

I was still testing the first step which is just whether the regular 'non responsive' preview button works, on top of this existing code. (Layout builder related code plus some others are not in my first commit yet.)

The preexisting code also uses the LupusDecoupledPreview class for modifying the 'regular' preview URL.

My interpretation of the pushed preexisting code is:

  • LupusDecoupledPreview explicitly modifies all 'regular preview' URLs to be node/preview/.... This is almost the same as the existing Core situation but the (only effective?) change is for preview URLs of nodes that are not yet changed. (i.e. immediately pressing "preview" in the edit screen.) Core has the normal, sometimes aliased (?? doublecheck) path for those preview routes.
  • The (I believe only?) reason for this explicit modification is, probably, so that the existing lupus_decoupled_ce_api LupusPreviewPathProcessor can easily attach "?auth-1" to the path.
  • Preview on this modified path for the unmodified node currently does not work because it needs to have the current state of the previewed entity saved in PrivateTempStore. This is not the case when previewing an unmodified node.
  • (Last time I tested, it did work in the system I am porting the code from. Noone knows why, from the top of their head.)

Update: I managed to get all the above points wrong. Please ignore.

I've already received a review of the general idea, and:

This should be renamed from lupus_decoupled_preview to drupal_decoupled_responsive_preview. The above 'regular node preview' should be moved into lupus_decoupled_ce_api.

It was suggested to do as little modification of Core functionality as possible. That includes trying to not override this 'preview of unchanged node' path. I am not sure yet what implications that has for LupusPreviewPathProcessor which should, AFAIK (new here), still add ?auth=1 to the URL.

roderik’s picture

Current status: 'regular preview' has been moved into lupus_decoupled_ce_api (in the same PR).

There's basically one effect of this added code: the preview redirects to the 'correct' front end, if there are multiple front ends.

(Secondary effect: I actually understand existing code, and which functionality is covered by which parts.)

I believe that now, the // Node edit form preview url. block in getPreviewUrl() is unnecessary. It used to be called from the 'Preview' form submit which has been removed in https://git.drupalcode.org/issue/lupus_decoupled-3320802/-/commit/cd667f....

roderik’s picture

This should be reviewable now;

At several points yesterday, I needed to apply #3395551: FrontendRedirectSubscriber should protect against being hijacked by 'destination' to fix the following:

  • From the node list I navigate to an edit page: /node/edit?destination=admin/content
  • I click the preview functionality in the toolbar
  • I see admin/content in the preview overlay, instead of the node.

...however I can neither reproduce it anymore at the moment, nor reason why/how the ?destination would be propagated into the preview. So I'm leaving it that out until I see it again.

fago’s picture

Status: Needs review » Needs work

Thx. Generally the changes seem good, but I think the code organization needs little work. See my above remarks.

> The /layout-preview route can always be there when the module is active, I think that makes sense anyway.

So code for that route should be moved into the layout_builder sub-module, it's not related to responsive preview really, but a generally layout-builder preview route which responsive preview then uses.

ad #3395551: FrontendRedirectSubscriber should protect against being hijacked by 'destination': not sure, but if it's not popping up again I'd agree to not handle it here

roderik’s picture

All validation errors should be gone when #3402425: Fix PHPCS and maybe other errors is merged and this is rebased upon it. (Test error still present, as noted there.)

roderik’s picture

Status: Needs work » Needs review
arthur_lorenz’s picture

Status: Needs review » Needs work

I found a couple of small issues in the MR

roderik’s picture

  • addressed review comments
  • Rebased -- now we can almost rely on Gitlab CI test results
  • fixed PHPstan (I hope)
  • sneaked in a random fix to lupus_decoupled_cors.info.yml that has nothing to do with this issue except it made me do a wrong copypaste last weeek (reviewed/caught by @fago)

The PHPUnit error is also present in other issues' builds so that needs to be addressed separately; there's an issue already open for that.

(Waiting for final test results - the latest test build isn't super fast.)

roderik’s picture

Update: latest commit shows PHPstan errors related to not being able to find responsive_preview module:
Drupal\lupus_decoupled_responsive_preview\LupusDecoupledResponsivePreview extends unknown class Drupal\responsive_preview\ResponsivePreview.
https://git.drupalcode.org/issue/lupus_decoupled-3320802/-/jobs/379502

So apparently the "test_dependencies:" (whose exact defined use I don't know) does not solve this.

And I'm reinstating the require-dev of drupal/responsive_preview, in order to make tests pass. I believe that require-dev is a good use for that and the module will still not be a requirement on 'regular' site installs.

roderik’s picture

Status: Needs work » Needs review
arthur_lorenz’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, testing previews on a fresh D10.1 with our nuxt kickstarter frontend was successful.

Tested:

  • Preview Button in node form
  • Responsive Preview modal in node form
  • Responsive Preview modal in layout form

  • fago committed ef37f275 on 1.x authored by roderik
    Issue #3320802 by roderik, arthur_lorenz: Add responsive preview...
fago’s picture

Status: Reviewed & tested by the community » Fixed

Merged, thank you!

roderik’s picture

Status: Fixed » Needs work

This is so embarrassing (and short) that I'm not going to open a new issue, but instead try to open a new MR here:

While fixing PHPCS/Stan errors.suggestions I copypasted wrong and replaced

  • strpos($url, '_') !== FALSE with str_contains($url, '_')
  • strpos($part, '_') !== FALSE with str_contains($url, '_') <======

...in ContentSecurityEventSubscriber.

As a result, a hostname containing an underscore in the first part e.g. https://temp_env.site.com is turned into https://*.*.* (instead of https://*.site.com) in the CSP header and your browser says "haha nice joke, I'm not displaying that frame"
.

  • fago committed e23df8ff on 1.x authored by roderik
    Issue #3320802 by roderik: Small fix for responsive preview...
fago’s picture

Status: Needs work » Fixed

ok, strange this did not pop up during testing? Merged.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.