Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make teaser block dynamic #6023

Closed
3 tasks done
pbauer opened this issue May 16, 2024 · 28 comments
Closed
3 tasks done

Make teaser block dynamic #6023

pbauer opened this issue May 16, 2024 · 28 comments
Assignees
Milestone

Comments

@pbauer
Copy link
Sponsor Member

pbauer commented May 16, 2024

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Philip Bauer @pbauer

Seconder: David Glick @davisagli

Abstract

Separated from #4208
Make teasers show dynamic data by default. Optionally users can choose to overwrite the data (old behavior)

Motivation

A teaser block currently does not change when the target-object is changed. This is breaking the expectations of most users.

Assumptions

Proposal & Implementation

All teasers are by default dynamic.
The teaser block gets a checkbox that allows to override a teaser. Checking it shows the teasers-fields that are read from the target.
The override-feature can be disabled globally in the volto registry.

A serializer in plone-restapi for the teaser block gets the data of the target to display if the block is not set to overwrite. So no additional requests are necessary to get the dynamic data on a lite with many teasers.

Extending the data is possible by adding a adapter for IJSONSummarySerializerMetadata. We could also try to configure that in the frontend by storing what is now selectedItemAttrs in the data of each teaser and using that in the serializer to add additional metadata.

Migration: We should prevent existing teaser to change only if you update Plone (volto and restapi). With the implementation in plone/plone.restapi@fb62f80 a teaser that was added before the overwrite-bool was added to the schema will be overwritten until the editor changes that.

The implementation is built based on #4790 by @steffenri, @nileshgulia1, @claudiaifrim and @pbauer and the block-serializer of @davisagli

Deliverables

Risks

The change in plone.restapi must not have any effect if used with a older version of Volto. That is already the case.
The change in Volto only works when used together with the updated version of restapi. I guess that is acceptable.

Participants

@pbauer
@pnicolli

@tisto
Copy link
Sponsor Member

tisto commented Jun 15, 2024

PLIP review 2024-06-15 Timo Stollenwerk (Volto Team Member, Framework Team Member)

Tested

  • create teaser from existing content object, update title/description/image on target, reflected on teaser -> works
  • create teaser from existing content object, choose "override source content", updated title/description/image on target, no change on teaser -> works
  • push "reload content" on existing teaser, reloads content from target -> works
  • uncheck "override source content", target content is used again -> works

Change Request

I noticed a wording inconsistency:

Edit-Plone-Site (5)

We agreed on "target", if I am not mistaken. Therefore I'd suggest using "target" and not "source" in the checkbox label and description.

Review

I suggest merging this PLIP after the label "change request" mentioned above has been decided/fixed.

@tisto
Copy link
Sponsor Member

tisto commented Jun 15, 2024

@sneridagh I added a formal review of this PLIP. I'd suggest that from now on we should have at least two formal reviews from Volto team members before we merge a PLIP. We need this formal process IMO.

This PLIP is also missing a seconder. Maybe @davisagli wants to second this?

@davisagli
Copy link
Sponsor Member

@tisto I added myself as seconder.

@stevepiercy
Copy link
Collaborator

I'm coming late to the party, so I have fresh eyes and now see the proposed UI.

I agree with @tisto to use target instead of source.

I think the checkbox label and its help text can be more explicit of what this feature does. But first, here is my understanding of how this new feature works.

  • When it is not checked, the displayed teaser content updates.
  • When it is checked, the displayed teaser content retains the original content.

Assuming that is accurate, then I would propose the following wording changes.

[ ] Display original content

When checked, the displayed teaser content retains the original content. When unchecked, a teaser displays updates to its content.

If that's too verbose, then we can drop the second sentence. Or maybe we keep verbosity in its first release, then drop it in a subsequent release.

@sneridagh
Copy link
Member

@davisagli @tisto @stevepiercy I can make the changes right away, if we decide on the final outcome.
Steve's suggestions are ok for me.

@tisto
Copy link
Sponsor Member

tisto commented Jun 16, 2024

@stevepiercy thanks for your suggestions! It is always very helpful to get feedback from someone who first sees a new feature.

Your assumptions are correct:

  • When it is not checked, the displayed teaser content updates. -> this means that the teaser updates automatically when the target object changes.
  • When it is checked, the displayed teaser content retains the original content. -> this means the teaser content "override" stays the same, no matter if the target content object is changed.

--

[ ] Display original content

When checked, the displayed teaser content retains the original content. When unchecked, a teaser displays updates to its content.

When I read "Display original content" in my head "original" means target/origin. Therefore checking this checkbox would mean that the content is automatically updated when the target/origin changes. This is the opposite of how it works.

Therefore "Override target content" seems more suitable to me. However, maybe I misunderstood the assumptions you explained above.

@stevepiercy
Copy link
Collaborator

@tisto I think it is a language translation issue. Perhaps instead of "original" we should use "initial"? In other words, the first content upon creation of the teaser block.

@sneridagh
Copy link
Member

@stevepiercy @tisto I tried to implement the suggestions, discussed it with @polyester too about it, we came up with this proposal:

image

What do you think? I have it there, just tell me and I'll push it.

@stevepiercy
Copy link
Collaborator

Override the target content with what? That's essentially the problem. Don't assume that users will just "get it". Maybe this is more explicit?

[ ] Display initial content

When checked, the teaser content displays the initial content. When unchecked, the teaser displays updates to its content.

@tisto
Copy link
Sponsor Member

tisto commented Jun 16, 2024

I get @stevepiercy's point here. "Override" is something that we get as developers and that makes sense to us, because we "override" the sync mechanism. For editors, it might be a different story. However, I am not able to come up with a better proposal ATM.

@tisto
Copy link
Sponsor Member

tisto commented Jun 16, 2024

Just thinking out loud here. What about:

  • title: "Keep initial content"
  • description: "Keep initial content, even when the content of the target object changes"

@sneridagh
Copy link
Member

@stevepiercy Override is related to everything that comes from the target, thus, just "Override" makes sense, because you totally override the current target content with the values below.

@tisto For me, it's not "initial", its the actual content of the target, "initial" has time implications. So "initial" for me leads to confusion. It has no answer for the question: Initial related to what? and expecting that later another event will have to be happened.

@polyester
Copy link
Sponsor Member

@tisto "Initial" to me would mean the state that the target content was in when the Teaser was created, which is exactly what the Live Teaser does not do.

So, my suggestion would be:

  • Title: "Customize teaser content"
  • Description: "By default, the teaser will show the relevant information from the target content item. You can customize this, which also means that the teaser content will not be updated when the target content is updated"

@tisto
Copy link
Sponsor Member

tisto commented Jun 16, 2024

@polyester sounds good to me!

@stevepiercy
Copy link
Collaborator

I like @polyester's version concept, but it should indicate what both checking and unchecking the box does. Also "By default" from the editor perspective might be different from what the programmer has configured globally.

  • Title: "Don't synch teaser content"
  • Description: "When unchecked, the teaser will display information synchronized with the target content item, updating its display when you change the target content. When checked, the display of teaser content will not synchronize when you update the target content."

Taking a step back, I have to wonder as an Editor, why would I ever edit target content when the box is checked? It's pointless to me. If I want the content to remain the same, then don't edit it. Did I miss some obvious use case?

That in turn begs the question, why even offer this option to Editors, and instead make this a hard breaking change?

@davisagli
Copy link
Sponsor Member

davisagli commented Jun 17, 2024

@stevepiercy I expect that editors will leave the box unchecked (use live data from the target) 95% of the time. The use case for checking it is if you want to tweak the presentation of a content item to better fit a specific context where it's being teased ... i.e. maybe to shorten the title to fit the available width, or edit the description or image to emphasize a different aspect of the item compared to other locations.

We also need the checked option in order to provide a smooth transition for existing teaser blocks. (As an editor I would not want to upgrade to a version of Plone that included this feature if it meant that all my teaser blocks would suddenly update in hard-to-predict ways.)

You bring up some valid critiques of @polyester's version. Here's my suggestion trying to incorporate parts of both of yours:

  • Title: "Customize teaser content"
  • Description: "Check this box to customize the title, description, or image of the target content item for this teaser. Leave it unchecked to show updates to the target content item if it is edited later."

@nileshgulia1
Copy link
Member

ATM either of override or customize works, provided there is a clear description. Ultimately at the end that checkbox will expose new fields to let the user handle title,desc, and image with their requirements. I'm slightly more inclined towards using the word "customize".

@polyester
Copy link
Sponsor Member

@stevepiercy the use case actually happens quite often. The target may have the actual title, image, description to make sense in for instance a news overview, but my teaser on the front page of my site might want to use a different set of fields to indicate a call-to-action, like "sign this petition NOW" or "you can still stop this horrible thing from happening".

As for the text on the checkbox, I remain in favour of a shorter version. My observation is that humans tend to zone out if a description is more than a few words, and do not actually read it.

@sneridagh
Copy link
Member

@davisagli

Check this box to customize the title, description, or image of the target content item for this teaser

what if your teaser has more fields at some point, because you extended it?

@sneridagh
Copy link
Member

Let's this not be a blocker, I think we put already far too many options on the table, and in this case, I'd go for keeping it simple.

@nileshgulia1
Copy link
Member

PLIP review 2024-06-17 Nilesh Gulia (Volto Team Member)

Tested with plone.restapi 9.6.1 and volto's main branch

  • checkout plone.restapi v9.6.1 locally.
  • Checked out volto's "main" branch which doesn't have this PR.
  • pnpm start
  • Created a normal page(a-page) and added a preview image.
  • Created a teaser block ( named as teaser-page-old) targeting the page created above.
  • Save the page.

Tested with plone.restapi latest and teaser-dynamic-again branch

  • checked out PR Make dynamic/live teaser without additional requests #6024
  • run "make start-backend-docker"
  • run "pnpm start"
  • Going to /teaser-page-old and seeing "overwrite: true" in blockData.
  • Updated the target page(a-page) preview_image and title.
  • Going to /teaser-page-old reflects the changed image but the "title" remains unchanged, with "overwrite: true" in the block data.
  • Created another page with teaser block ( named as teaser-page-new) targeting the page(a-page) above.
  • On inspecting the block data, the field "overwrite: false" was there.
  • Updated the target page(a-page) preview_image and title -> the image and title reflects on /teaser-page-new.
  • with the overwrite option checked, if there is no image_override, the image still keeps updating with the target page.
  • In the /teaser-page-new, customized title,desc and added override image -> works as intended.
  • Clicking refresh source content -> fetches latest image and title as intended.

Reviews/Observation

In the page with existing teasers before this feature, the overwrite: true has been added. When this option is checked and if we don't have image_override yet, it continues to fetch the updated image, but with no title as we chose to customize it. This is intended behaviour. I suggest this PLIP can be merged.

Changes requested

No change is requested.

@stevepiercy
Copy link
Collaborator

Thanks everyone! I missed those use cases. With that, I think @davisagli's version encompasses all of our concerns.

@davisagli
Copy link
Sponsor Member

what if your teaser has more fields at some point, because you extended it?

@sneridagh Then you can customize the text for the checkbox in the same extender.

@sneridagh
Copy link
Member

@davisagli ooookkkkkk!

image

After the agreement, ready to merge then.

@stevepiercy
Copy link
Collaborator

@stevepiercy
Copy link
Collaborator

Docs added in a8a7dbb

@davisagli
Copy link
Sponsor Member

@stevepiercy Thank you! 🌟

@pbauer
Copy link
Sponsor Member Author

pbauer commented Jun 19, 2024

Thanks to everyone who contributed to this great feature!

@pbauer pbauer closed this as completed Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

8 participants