Skip to content
This repository has been archived by the owner on Jul 27, 2022. It is now read-only.

ISAICP-6575: Search result index new view modes (search_result_featured, search_result). #2541

Open
wants to merge 21 commits into
base: EPIC-6572
Choose a base branch
from

Conversation

saidatom
Copy link
Contributor

@saidatom saidatom commented Aug 9, 2021

No description provided.

@saidatom saidatom changed the base branch from ISAICP-6415 to ISAICP-6688 October 6, 2021 16:04
Base automatically changed from ISAICP-6688 to EPIC-6572 October 13, 2021 16:16
@pfrenssen
Copy link
Contributor

pfrenssen commented Oct 22, 2021

I started with a manual test.

The 'Video' and 'Licence' bundles are not appearing styled as a card UI pattern. They both seem to have the right view mode selected in the View, but the template file is missing, so the video is rendered using the standard node.html.twig template and the licence using rdf-entity.html.twig

A comment made by Theo on the Jira ticket contains details about how the UI pattern should be configured. I don't see this appearing in the styling. For example the following specification would cause the content to be split in a small left column and a wide right column, but I don't see this on the screen:

        horizontal_grid: {
          left_col_classes: "col-md-3",
          right_col_classes: "col-md-9",
          gutter: 0,
        },

Comment on lines 7 to 14
{% block content %}
{{ pattern('card', {
'variants': 'horizontal',
'url': url,
'title': { 'content': label },
'content': content,
}) }}
{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that many of these templates are identical. We can deduplicate them by "base templates" that can be used by all nodes and RDF entities:

  • node--search-result.html.twig
  • rdf-entity--search-result.html.twig

Then in case there are any bundles that need to have a custom template for some reason, we can still override the base template by defining node--{bundle}--search-result.html.twig.

Comment on lines 12 to 13
'content': content|without('field_news_logo'),
'image': content.field_news_logo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom page bundle doesn't have the field_news_logo field, this is probably forgotten to be updated after copy/pasting from the News template.

This indicates that we are missing test coverage for this. I don't know how these "featured" search items are supposed to be visible? Is this already implemented? If we can already see them in some way then we should add a test that ensures the right image is shown for each bundle that can appear as a featured search item.

Comment on lines 12 to 13
'content': content|without('field_news_logo'),
'image': content.field_news_logo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a discussion, so there is no field_news_logo, same as above. Let's double check this for all bundles.

Comment on lines 12 to 13
'content': content|without('field_ar_logo'),
'image': content.field_ar_logo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this image field is the only thing that varies across all bundles, then we can solve this in a cleaner way in a preprocess function rather than providing a unique template for each bundle.

We can remove the image from the view mode in the field UI, then it will no longer be part of it.

Then we replace this section in the template with:

    'content': content,
    'image': image,

Now we just need to provide the image variable but this can be done cleanly in a preprocess function, it might look something like this:

function ventuno_preprocess_rdf_entity__search_result(&$variables) {
  $entity = $variables['rdf_entity'];
  $variables['image'] = $entity instanceof LogoInterface ? $entity->getLogoAsRenderArray() : NULL;
}

Copy link
Contributor

@idimopoulos idimopoulos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to Pieter's comments, there is also a test failure. The test failure seems to derive by the fact that the new view modes, "search result" and "search result featured" do not seem to display contextual links at the moment (configuration miss?). Please, fix the test as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants