Add options to override listing blocks "no results". Add a default noResultsComponents, and another for the imageGallery variation

ionlizarazu commented 1 year ago

add the option to modify via config registry the "No results found" message of the Listing block as component
add the option to have a different "No results found" message for each variation
added as example a different "No results found" message for the ImageGallery variation resolves #3552

netlify[bot] commented 1 year ago

Deploy Preview for volto ready!

Name	Link
Latest commit	f8f740f22c7e932220bb757d2e0c936da2e4988a
Latest deploy log	https://app.netlify.com/sites/volto/deploys/63fc7457d2dc2b0008b8f2de
Deploy Preview	https://deploy-preview-3602--volto.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

cypress[bot] commented 1 year ago

Passing run #4094 ↗︎

0

459

20

0

Details:

Update docs/source/blocks/index.md
Project: Volto	Commit: `f8f740f22c`
Status: Passed	Duration: 14:53 💡
Started: Feb 27, 2023 9:17 AM	Ended: Feb 27, 2023 9:32 AM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

erral commented 1 year ago

IMHO to include a component in the variation to show the No results component does not work. It difficults reusing variations.

Say I want to prepare a variation to use it both in News listing and Event listings, and in one I want to put "no future events" and in the other "there are no news".

Using a single component to add this message would mean to add a new variation just to change the "No results".

My 2 cents.

ionlizarazu commented 1 year ago

I've added some data to the NoResults component, so now we can enhance the schema for the variation and use the values in the NoResults component.

For @erral example:

You extend the schema for the variation with a field (no_results_text)
Create your own NoResultsComponent showing the no_results_text data
Configure your NoResultsComponent for your extended variation

ionlizarazu commented 1 year ago

It makes sense, do you have any example in other component to see how to do it? @sneridagh

sneridagh commented 1 year ago

@ionlizarazu

Not yet a lot of examples but:

https://github.com/plone/volto/blob/846b095a26e19bdc615dd9b1fecd7e4917001d45/src/components/theme/PreviewImage/PreviewImage.jsx#L15-L20

We are using it a lot lately:

Select which teaser template you should use based on the content type:

  const hasType = data.href?.[0]?.['@type'];

  const BodyComponent =
    (config.getComponent &&
      hasType &&
      config.getComponent({ name: 'Teaser', dependencies: [hasType] })
        .component) ||
    variation?.template ||
    DefaultBody;

and the registration:

  config.registerComponent({
    name: 'Teaser',
    component: NewsItem,
    dependencies: 'News Item',
  });

hope this helps!

ionlizarazu commented 1 year ago

so, shoud I register the component using registerComponent function or just writing down here: https://github.com/plone/volto/blob/master/src/config/Components.jsx

sneridagh commented 1 year ago

@ionlizarazu better the register component... think of it as the API to do the registrations. Like ZCA. ;)

Could be that to avoid confusion we change the registration of PreviewImage in there.

erral commented 1 year ago

I have added documentation on how to use this feature

tiberiuichim commented 1 year ago

@sneridagh let's merge this, it makes sense to have it

erral commented 1 year ago

@sneridagh can you look at this please?

erral commented 1 year ago

@sneridagh changelog and documentation changed. Thanks for looking at this!

sneridagh commented 1 year ago

@erral could you weigh on @ksuess comment?

erral commented 1 year ago

I think that's possible of course, let me check it with @ionlizarazu

ionlizarazu commented 1 year ago

I think that including the HeadlineTag in the variation and the noResultsComponent would be a breaking change, because it would break all the existing variations. On the other hand, to give the possibility to hide the headline, I would add another field to the listing schema to decide to hide the heading when there are no results instead of adding a css class and force the user to develop a theme for the personalization. I think the description can be something like "Hide heading when there are no results"

@erral @sneridagh @ksuess

ksuess commented 1 year ago

I'm stepping back from my suggestion to include the heading in the NoResultsComponent.

I am in favor of doing both:

your introduction of a NoResultsComponent per block and a NoResultsComponent per variation
my introduction of a CSS marker class https://github.com/plone/volto/pull/4394 These can be combined. The introduction of a marker class does not break existing custom variations, as it does leave the ListingBodyTemplate(variation) intact. The heading stay where it is, outside of the ListingBodyTemplate.

In addition to the suggested changes in https://github.com/plone/volto/pull/4394 from yesterday, I added another commit. With this it is now possible to hide a listing with no results completely independent of variations. And it is possible to hide a listing with no results completely only for some variations.

Just to not forget, I like your solution for the option of overriding the no-resuts text with a schema enhancement and custom no-rsults component by passing the data to <NoResults isEditMode={isEditMode} {...data} />

erral commented 1 year ago

@ksuess and what about adding an option in the schema to hide the heading or the listing when there are no results? That's easier from the CMS user perspective: just hide the title and it's done. No need to add CSS code to achieve it.

ksuess commented 1 year ago

@ksuess and what about adding an option in the schema to hide the heading or the listing when there are no results? That's easier from the CMS user perspective: just hide the title and it's done. No need to add CSS code to achieve it.

My request that I have to solve is: "Hide a listing if there is no result". This is easy with a .emptyListing { display: none;) Instead it is a pain for an editor to visit all 1017 listings and check the "hide if there are no results". Do you see may point now?

erral commented 1 year ago

I see 😅

erral commented 1 year ago

@sneridagh then we can merge this one and then @ksuess' PR

erral commented 1 year ago

Thanks @stevepiercy for the review. I have reverted all those quotation changes.

I really don't know why my editor does this kind of stuff... I am wondering if our .estlintrc configuration may have some influence on that... I will check it.

plone / volto