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

OEL-1017: [oe_whitelabel] Style the news content type #68

Merged
merged 23 commits into from
Feb 18, 2022
Merged

Conversation

GilNovacomm
Copy link
Contributor

Create oe_whitelabel_news submodule.
Create date format.
Configure full and teaser displays for the News content type.
Update required dependencies.
Create rendering tests.

@GilNovacomm GilNovacomm force-pushed the OEL-1017 branch 2 times, most recently from 776084e to a5009e6 Compare February 10, 2022 14:51
…d ctools due to drupal 9.2 compatibility on lowest installation.
composer.json Outdated
@@ -6,29 +6,34 @@
"minimum-stability": "dev",
"prefer-stable": true,
"require": {
"php": ">=7.3",
"php": ">=7.4",
"cweagans/composer-patches": "^1.7",
"drupal/core": "^8.9 || ^9.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

We change to PHP 7.4 and don't change for drupal 9.2?

type: module
description: Adds additional functionality to the News module
package: OpenEuropa Whitelabel Theme
core_version_requirement: ^8.9 || ^9.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Drupal 9.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I've confirmed with Francesco we should update to D9.2

* OE Whitelabel theme News.
*/

declare(strict_types=1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
declare(strict_types=1);
declare(strict_types = 1);

{% endset %}
{{ pattern('card', {
variant: 'search',
title: label,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
title: label,
title: _title,

"drupal/drupal-extension": "~4.1",
"drupal/file_link": "^2.0.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

why is needed?

  • file_link, pathauto, email_validator?

Copy link
Contributor Author

@GilNovacomm GilNovacomm Feb 11, 2022

Choose a reason for hiding this comment

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

This issue is due to when installing the lowest versions:
Downgrading drupal/core (9.2.12 => 9.2.0): Loading from cache
these modules do not have the core_version_requirement key on their info file.
An explanation to each is on the _readme key on composer.json
"_readme": [ "Explicit minimum version requirement of drupal/ctools module due to D9.2 compatability.", "Explicit requirement for drupal/file_link due to https://www.drupal.org/project/file_link/issues/3147517. It can be removed when oe_media requires version 2.0.4 or above.", "Explicit requirement for drupal/pathauto due to D9.2 compatability according to https://www.drupal.org/node/2979476.", "Explicit requirement for egulias/email-validator due to https://www.drupal.org/project/drupal/issues/3061074#comment-14300579. It can be removed when Drupal core 9.2 support is droppped." ],

* News full display.
*/
#}
{% set badges = [] %}
Copy link
Contributor

Choose a reason for hiding this comment

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

we need that? We can put badges: badges|render|default([])??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed it will clean the template and remove an unused array.

* Search result template.
*/
#}
{% set badges = [] %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same above

'#value': content.oe_publication_date|render|striptags,
},
badges: badges|render
}) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add EOL

@@ -0,0 +1,7 @@
langcode: und
Copy link
Contributor

Choose a reason for hiding this comment

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

How come this is und? Usually it's en

langcode: und
status: true
dependencies: { }
id: oel_whitelabel_news_date
Copy link
Contributor

Choose a reason for hiding this comment

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

oe_, not oel_.

@@ -0,0 +1,13 @@
name: OpenEuropa Whitelabel News
type: module
description: Adds additional functionality to the News module
Copy link
Contributor

Choose a reason for hiding this comment

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

The scope of the module is not to add additional functionality, but it's a companion module to provide the styling.

modules/oe_whitelabel_news/oe_whitelabel_news.module Outdated Show resolved Hide resolved
tests/src/Functional/ContentNewsRenderTest.php Outdated Show resolved Hide resolved
tests/src/Functional/ContentRenderTestBase.php Outdated Show resolved Hide resolved
tests/src/Functional/ContentRenderTestBase.php Outdated Show resolved Hide resolved
tests/src/Functional/ContentNewsRenderTest.php Outdated Show resolved Hide resolved
tests/src/Functional/ContentNewsRenderTest.php Outdated Show resolved Hide resolved
escuriola
escuriola previously approved these changes Feb 17, 2022
@brummbar brummbar merged commit f228bc4 into 1.x Feb 18, 2022
@brummbar brummbar deleted the OEL-1017 branch February 18, 2022 17:06
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.

3 participants