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

EWPP-287: Create organisation teaser #595

Merged
merged 3 commits into from
Sep 25, 2020
Merged

EWPP-287: Create organisation teaser #595

merged 3 commits into from
Sep 25, 2020

Conversation

yenyasinn
Copy link
Contributor

EWPP-287

Teaser for Organisation node.

@@ -0,0 +1,61 @@
langcode: en
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2020-09-24 at 19 40 02

current teaser view

Copy link
Contributor

@drishu drishu left a comment

Choose a reason for hiding this comment

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

The approach looks good to me, I like the adjacent fixes, just 2 small copy paste errors found.

use Drupal\user\Entity\User;

/**
* Tests the project rendering.
Copy link
Contributor

Choose a reason for hiding this comment

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

Organisation rendering

}

/**
* Test a project being rendered as a teaser.
Copy link
Contributor

Choose a reason for hiding this comment

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

Test a organisation being..

drishu
drishu previously approved these changes Sep 25, 2020
{{ pattern('list_item', {
'variant': 'thumbnail_secondary',
'url': url,
'meta': content.oe_organisation_acronym|field_value,
Copy link
Member

Choose a reason for hiding this comment

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

meta should accept an array, change it into:

 'meta': [
  content.oe_organisation_acronym|field_value,
]

'additional_information': [
content.extra_field_oe_theme_content_organisation_teaser_details,
],
'image': image_url ? { 'src': image_url }
Copy link
Member

Choose a reason for hiding this comment

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

Should we not pass the alt value of the image here?

Copy link
Member

Choose a reason for hiding this comment

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

We are not doing that anywhere actually, as the image is displayed as a background anyway and we want to refactor that into using media container. I'd postpone this to this follow up: EWPP-249 which has already some POC work done here #569

use Symfony\Component\DependencyInjection\ContainerInterface;

/**
* Display contact information.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Display contact information.
* Display organisation details.

return $build;
}

$cache->addCacheableDependency($contact);
Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe move this before line 89? Changes on the entity might have a consequence on its access.

protected function getRenderableFieldListItem(ContentEntityInterface $entity, string $field_name, array $display_options = []): array {
if ($entity->get($field_name)->isEmpty()) {
return [];
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this if before 109, so we only check for the empty field and we don't have to check for if (!empty($item)) {. It will be the responsibility of the called to call this function knowing that it will return.

}

/**
* Gets renderable item for field list pattern.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Gets renderable item for field list pattern.
* Get renderable item for field list pattern.

@ademarco ademarco merged commit 6bdcabe into EPIC-Organisation Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants