plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
251 stars 188 forks source link

PLIP: views or viewlets for title and description #2740

Closed iham closed 5 years ago

iham commented 5 years ago

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Markus Hilbert @iham

Seconder: Jens W. Klein @jensens

Abstract

make title/description more flexible to customise and move them to viewlets in order to do so.

Motivation

whenever some dev somehow wants to customise the title and/or description (e.g. richtext) they might need to subclass the MainTemplate class in order to get the template registered and alter the main_template.pt to fit to there needs. ... which is not the best idea at all

Assumptions

being able to customize/subclass title and description with more ease.

Proposal & Implementation

in order to keep the metal-slot as they are used a lot, there must be a viewletmanager inside those slots.

in plone.app.layout.viewlets registration would look like:

    <browser:viewletManager
        name="plone.contenttitle"
        provides=".interfaces.IContentTitle"
        permission="zope.Public"
        class="plone.app.viewletmanager.manager.BaseOrderedViewletManager"
        />

    <browser:viewletManager
        name="plone.contentdescription"
        provides=".interfaces.IContentDescription"
        permission="zope.Public"
        class="plone.app.viewletmanager.manager.BaseOrderedViewletManager"
        />

    <browser:viewlet
        name="plone.title"
        manager=".interfaces.IContentTitle"
        template="title.pt"
        permission="zope2.View"
        />

    <browser:viewlet
        name="plone.description"
        manager=".interfaces.IContentDescription"
        template="description.pt"
        permission="zope2.View"
        />

title.pt

<h1 class="documentFirstHeading"
  tal:define="title context/Title"
  tal:condition="title"
  tal:content="title">Title or id</h1>

description.pt

<div class="documentDescription description"
    tal:define="description context/Description"
    tal:content="description"
    tal:condition="description">
  Description
</div>

BTW: i would love to change that div to a paragraph to improve semantics

main_template.pt in products.cmfplone

<header>

  <div id="viewlet-above-content-title" tal:content="structure provider:plone.abovecontenttitle" />

  <metal:title define-slot="content-title">
    <div id="viewlet-content-title" tal:content="structure provider:plone.contenttitle" />
  </metal:title>

  <div id="viewlet-below-content-title" tal:content="structure provider:plone.belowcontenttitle" />

  <metal:description define-slot="content-description">
    <div id="viewlet-content-description" tal:content="structure provider:plone.contentdescription" /> 
  </metal:description>

  <div id="viewlet-above-content-body" tal:content="structure provider:plone.abovecontentbody" />

</header>

Deliverables

customize Products.CMFPlone's main_template.pt add ViewletManagers to plone.app.layout add title and description templates and register them as viewlets to the managers.

Risks

as this implementation keeps the metal slots, there should be no real problem or breakage.

compute-time to render the template might be different, but i don't know that for sure.

as this implementation spreads two packages (cmfplone and p.a.layout) this might take some time to become released "together"

maybe plone.app.standardtiles and/or plone.app.mosaic can conflict, but on first sight it doesn't look like that.

Participants

@iham

iham commented 5 years ago

reconsidering this a little ...

why using viewlets and managers - i don't see no need for that anymore

so ... here is a different approach:

title.pt and description.pt stay the same but get registered as views aka pages:

  <browser:page
    for="*"
    name="title"
    template="title.pt"
    permission="zope2.View"
    />

  <browser:page
    for="*"
    name="description"
    template="description.pt"
    permission="zope2.View"
    />

and the main_template just renders those into the given metal-slots to keep them as mentioned above

<header>

  <div id="viewlet-above-content-title" tal:content="structure provider:plone.abovecontenttitle" />

  <metal:title define-slot="content-title">
    <div tal:replace="structure context/@@title" />
  </metal:title>

  <div id="viewlet-below-content-title" tal:content="structure provider:plone.belowcontenttitle" />

  <metal:description define-slot="content-description">
    <div tal:replace="structure context/@@description" />
  </metal:description>

  <div id="viewlet-above-content-body" tal:content="structure provider:plone.abovecontentbody" />

</header>

thats way easier and addresses the same problems: subclass-ability and customisation of those attributes - like a richdescription or a title/subtitle header group...

iham commented 5 years ago

i'd also add those simple views ONLY to products.cmfplone to avoid cross-referencing to plone.app.layout.

ale-rt commented 5 years ago

Is not simpler and cleaner to just have a 'plone.contentheader' (or something similar) viewlet manager where you can order viewlets (then you will have to add one for the title and the description) the way you want?

This will replace:

In this way we can fix this weird mixture between metal slots and viewlet.

jensens commented 5 years ago

@ale-rt I agree, but imo this is out of the scope of this PLIP.

iham commented 5 years ago

@ale-rt i basically hoped for the same, but the metal slots are still in heavy use and just to be honest, it is outside my reach to fix all those bits as i'd be hopping through packages for weeks.

my scope for this plip is to simple make the title and description customise-able not touching the main_template.

this can be achieved through both ways (views or viewlets with manager)

i already implemented the simple way and tests are passing. -> #2747 except for some weird robot-issue again ... any help on that?

jensens commented 5 years ago

After I now see the implementation at #2747 I am the opinion there is not really a formal PLIP needed. The implementation is that simple, I would say we could backport it to 5.1 if we want. Even already customized main-templates wont break.

So I am +1 merging it right away.

@ale-rt what do you think? @plone/framework-team ?

ale-rt commented 5 years ago

Well, if you are fine introducing, maintaining and documenting a couple of views for this purpose I am not against merging #2474 even if I am not enthusiast. My main issue is that we introduce yet another way of doing things (which is probably better and simpler than viewlets, BTW). If this new approach become mainstream we can have the proliferation of views for other main template elements too. The idea behind the plip is good but as mentioned I would profit of that to get rid of the mixture of slots and viewlet managers. Maybe this could be done for Plone 6.

jensens commented 5 years ago

@ale-rt I still agree with you, but if there is no one implementing it (and I fear it is some effort, also thinking of upgrade steps for it)? Maybe this is something for GSOC?

The current solution solves the problem that current code does not respect the behaviors adapters. So, to me, it feels more like a bugfix.

jensens commented 5 years ago

see merged #2747