Problem/Motivation

Since #3542003: Responsive viewports with iframes, the PreviewPanel include the page around the preview according to a renders_full_page attribute on Buildable plugins.

We use term Chrome as the UI framing around content: toolbars, tabs, borders, handles. Everything that is not the content itself.

It is a direct rewrite of the previous rules based on routing:

  • PageLayout: Preview only the display
  • EntityView, EntityViewOverride & Views: Preview is wrapped in a full page

However, we may need a more dynamic mechanism:

Wrapped in a full page? Only the preview?
PageLayout Never Always
EntityView & EntityViewOverride Only for the full display (or default is full is not activated) of the fieldable entities with full page rendering (node, user, term, sometimes media...) For all other displays and other entities (block, comment, paragraph..)
Views Only for the page displays For other like block, embed & attachment displays

Proposed resolution

Maybe DisplayBuildableInterface::previewWithChrome(): bool method which is returning:

  • PageLayout: always false
  • EntityView & EntityViewOverride: true or false according to the entity type and the display ID
  • Views: true or false according to the ViewsDisplay plugin

For EntityView & EntityViewOverride, it is challenge to know which entity type have a full page display. The best approximate we know for now is to check canonical and edit-form entity links.
OK positive:

Content entity type Preview in a full page Different canonical & edit-form links canonical edit-form
Node ✅ ✅ /node/{node} /node/{node}/edit
Taxonomy Term ✅ ✅ /taxonomy/term/{taxonomy_term} /taxonomy/term/{taxonomy_term}/edit
User ✅ ✅ /user/{user} /user/{user}/edit
Media ✅/❌ ✅/❌ (depend of a config) /media/{media}/edit

False positive:

Content entity type Preview in a full page Different canonical & edit-form links canonical edit-form
Comment ❌ ✅ /comment/{comment} /comment/{comment}/edit

OK negative:

Content entity type Preview in a full page Different canonical & edit-form links canonical edit-form
Block Content ❌ ❌ /admin/content/block/{block_content} /admin/content/block/{block_content}
Message ❌ ❌ No No
Menu Link ❌ (display not buildable) ❌ /admin/structure/menu/item/{menu_link_content}/edit /admin/structure/menu/item/{menu_link_content}/edit
Shortcut ❌ (display not buildable) ❌ /admin/config/user-interface/shortcut/link/{shortcut} /admin/config/user-interface/shortcut/link/{shortcut}
Path Alias ❌ (display not buildable) ❌ No No
File ❌ (display not buildable) ❌ No No
Workspace ❌ (display not buildable) ❌ No No

So, we need to find a way of excluding Comment (without hardcoding, so we are also compatible with entity type from contrib & custom).

Remove renders_full_page attribute on Buildable plugins.

Issue fork display_builder-3615194

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:

About issue forks

Comments

pdureau created an issue. See original summary.

mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Issue summary: View changes
pdureau’s picture

pdureau commented
Issue summary: View changes
mogtofu33’s picture

mogtofu33 commented
Assigned: Unassigned » mogtofu33
Status: Active » Needs work

mogtofu33 opened merge request !362

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau
Status: Needs work » Needs review

For views we leverage DisplayRouterInterface instead of ViewsDisplay plugin. It allow page and feed, but we do not have our display plugin available on feed so it is not a problem.

For now media and comment do not have a specific code for their behavior, as discussed it is not worth at the moment, could be a follow up if required. Seems too specific to be included now.

Tested Block entity: always no chrome so seems ok. So probably good for other entities.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs review » Needs work

Code review

All good but... why ::previewsFullPage() is EntityDisplayLabelTrait? It seems a bit weird.

Feature review

Page Layout

OK ✅

Entity View / Entity View Override

For now media and comment do not have a specific code for their behavior, as discussed it is not worth at the moment, could be a follow up if required. Seems too specific to be included now.

I agree, no specific code. And we don't need for media, because the config switch works well

Entity type default when no full default when full full Others
Block ✅ no chrome ✅ no chrome ✅ no chrome ✅ no chrome
Node ✅ with chrome ⚠️ with chrome ✅ with chrome ✅ no chrome
Taxonomy term ❌ no chrome ❌ no chrome ❌ no chrome ✅ no chrome
Media (without "Standalone media URL") ✅ no chrome ✅ no chrome ✅ no chrome ✅ no chrome
Media (with "Standalone media URL") ✅ with chrome ⚠️ with chrome ✅ with chrome ✅ no chrome
User ✅ with chrome ⚠️ with chrome ✅ with chrome ✅ no chrome

2 feedback:

  • ⚠️Node, Media, User and others: default must be with chrome only if full display doesn't exist? Don't you think?
  • ❌ Taxonomy term: I don't know what is happening here, entity.taxonomy_term.canonical has _entity_view: 'taxonomy_term.full', maybe i am doing something wrong during my tests

Views

For views we leverage DisplayRouterInterface instead of ViewsDisplay plugin. It allow page and feed, but we do not have our display plugin available on feed so it is not a problem.

✅ It seems to work well

mogtofu33’s picture

mogtofu33 commented

why ::previewsFullPage() is EntityDisplayLabelTrait

⚠️Node, Media, User and others: default must be with chrome only if full display doesn't exist? Don't you think?

Splitted + added default with/without full condition.

❌ Taxonomy term: I don't know what is happening here, entity.taxonomy_term.canonical has _entity_view: 'taxonomy_term.full', maybe i am doing something wrong during my tests

Taxonomy term pages use a core View that overrides the canonical route, so the route no longer shows _entity_view, even though the view still renders the term's display via a header area. Our check only looks at the route, so it misses this case. Known limitation, ignored for now.

A very specific behavior it seems (again..) I think we can ignore for now and have a follow up if required.

Added the specific cases to the doc, at least we acknowledge them.

Added a floating island to inform current chrome and why. fixed some style issues on preview and scroll behavior.

pdureau’s picture

pdureau commented

A very specific behavior it seems (again..) I think we can ignore for now and have a follow up if required.

Sure, let's ignore for now.

mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » pdureau

PHPunit failure seems related to UI Patterns 2.0.20 and will mostly be fixed in #3618072: Placeholders and real render, in Canvas and Preview, not a blocker here.

pdureau’s picture

pdureau commented
Assigned: pdureau » mogtofu33
Status: Needs work » Reviewed & tested by the community

Tested, not as deep as the preview review, but seems OK. 👍

About the "Chrome indicator" island:

  • with chrome: ✅ the icon is great, the wording is good enough
  • without chrome: ⚠️ Just humble suggestions, not blocking the RTBC:
    • I would simply display nothing, because this is simply the expected display preview, without fancy stuff.
    • If we keep it, i would not use a puzzle icon here.
mogtofu33’s picture

mogtofu33 commented
Assigned: mogtofu33 » Unassigned
Status: Reviewed & tested by the community » Fixed

Agree, removed bare, only chrome is marked.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • mogtofu33 committed 5ee7e948 on 1.0.x 
    task: #3615194 Dynamic rules for wrapping in PreviewPanel

By: pdureau...

  • mogtofu33 committed 60d8e322 on 1.0.x 
    fix: #3615194 minor style fix on preview chrome

Status: Fixed » Closed (fixed)

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