Convert aggregator-item.html.twig to HTML5

should we approach this from the "each field in it's own container" strategy?

I don't remember which thread I was reading but the plan was suggested that we shouldn't contruct html5-based objects as follows:

<article>
  <header></header>
  <time pubdate="now">time here</time>
  <footer></footer>
</article>

but instead do this

<header></header>
<article>
  <time></time>
</article>
<footer></footer>

Is my assumption correct? is this the way we modify the aggregator-term.tpl.php?

What would the inclusion of the Header and Footer Tags be for?

Previously the most frustrating part of Drupal front-end development is the Tag soup that comes from over implementing things.

Because of HTML5's youth, it has the potential to be a labyrinthine system of Tags. Since some of the relationships of these new Tags can sometimes be in question, we have an opportunity here to implement clean code according to our standards.

Also your api link goes to aggregator-item.tpl.php and not aggregator-term.tpl.php. Which template are you meaning to reference?

I think this one should mirror the current node.tpl.php as much as possible.

This seemed like a good place for me to try and help with core.

I am new to core contrib, and the html5 specs can get confusing as they are still a moving target.

I am not sure if this is the best way to accomplish what we want but it might help to at least get some discussion going here.

Testing

Here is a version using theme_datetime().

This corrects the datetime value using format_date() but it feels like to much php inside of a template for my liking.

Guess I will wait for feedback on this.

This will need reworking now that the templates have been converted to Twig - especially the PHP inside the template, something closer to #6 would work better.

Let me check this one...

- Improved markup to HTML5
- Also improved structure, now more looks like node structure.

wow the code behind aggregator looks like its a loooongtime since it got some real love - loads of hardcoded variables n stuff.
Oooh well if fixed the issue with hardcode "time ago" translation & added in a variable for the date.
split up the catagories so we can do something with em in the template (instad of using implode)

Here is a re-roll of #12 with whitespace cleaned up.

1. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -16,28 +16,44 @@
   +    <div>
   

   are we gonna have any class on this - think it should follow the same naming as a node (but be easy to clean out)

2. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -16,28 +16,44 @@
   +      {{ sourcetime|date("Y-m-d H:i") }}
   

   we should be able to make a "X days ago" formating ?
   probably a preprocess

the categories data wasnt correct thats fixed now

* Why the change to a numeric array?
cause i like to be able to not have the twig vars look like this: {{ foo[key].title }}

        {% for item in categories %}
          {% if loop.last %}
          <a href="{{ item.href }}">{{item.title}}</a>.
          {% else %}
          <a href="{{ item.href }}">{{item.title}}</a>,
          {% endif %}
        {% endfor %}

Good call with the h3 & attributes

Removed some whitespaces in previous patch.

Some nitpicky coding standards review:

1. +++ b/core/modules/aggregator/aggregator.pages.inc
   @@ -74,19 +74,16 @@ function template_preprocess_aggregator_item(&$variables) {
   +  $variables['createdtime'] = $item->getPostedTime();
   
   +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   + * - createdtime: Date
   

   This variable should be called posted_time to match closer to what it's coming form.

2. +++ b/core/modules/aggregator/aggregator.pages.inc
   @@ -74,19 +74,16 @@ function template_preprocess_aggregator_item(&$variables) {
   +      'title' => $category->title ,
   

   Remove the space before the comma.

3. +++ b/core/modules/aggregator/aggregator.pages.inc
   @@ -74,19 +74,16 @@ function template_preprocess_aggregator_item(&$variables) {
   +      'href' => 'aggregator/categories/'.$category->cid
   

   Needs the space around the period.

4. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   + * - source_date: Date the feed was posted. formated as n days ago
   

   Formatted should be Sentence case and N should be capitalized. The sentence should end in a period.

5. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   + * - categories: categories assigned to the feed.
   

   Sentences should start with Caps.

6. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   +
   

   Remove this newline.

7. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   +    <div>
   ...
        </div>
   

   Do we need this div anymore?

8. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,36 +8,50 @@
   +          <a href="{{ item.href }}">{{item.title}}</a>.
   ...
   +          <a href="{{ item.href }}">{{item.title}}</a>,
   

   Theses needs spaces before and after the insides of the {{ }} and indented one more.

Will take over to make these changes...

Patch rerolled, changes are made...

@Dragan Eror, Did it need a re-roll? I don't see any of the changes from #19 in that patch.

Also this is probably a good opportunity to point out that we likely need to extend Twig's date format to use named formats from drupal.

+++ b/core/modules/aggregator/aggregator.theme.inc
@@ -31,14 +31,17 @@ function template_preprocess_aggregator_item(&$variables) {
-    $variables['source_date'] = format_date($item->getPostedTime(), 'medium');

+++ b/core/modules/aggregator/templates/aggregator-item.html.twig
@@ -8,29 +8,44 @@
+    <time datetime="{{ createdtime|date("Y-m-d H:i") }}">

This will likely need a separate issue and if someone's interested in creating it I'll help where I can to make this happen.
This should read like:
<time datetime="{{ createdtime|date_format("medium") }}"> Note date_format filter doesn't exist in twig. Or we some how merge date filter with date_format but that could get buggy/hacky quick...

@joelpittet don't need to re-roll. Only change I didn't make is that one with the date. Didn't want to make variable changes just because "nicer" name because now follows same name as node.html.twig.

Reroll

Thanks for the reroll @lauriii.

1. +++ b/core/modules/aggregator/aggregator.theme.inc
   @@ -39,6 +39,7 @@ function template_preprocess_aggregator_item(&$variables) {
   +  $variables['posted_time'] = $item->getPostedTime();
   
   +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,29 +8,41 @@
   + * - posted_time: Date the feed was posted.
   

   This doesn't seem to be there or being used, may have came from a reroll.

2. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
   @@ -8,29 +8,41 @@
   +<article class="feed-item {{ attributes.class }}">
   

   addClass() ftw:)

Ups, forgot to change variable used in the twig file. addClass() ftw ;)

A lot have happened in short amount of time regarding this little template. Basically there are 2 different issues that have already modified this template and it's data alot.

1. #2261425: Streamline aggregator's entities rendering with rest of core (3 months ago) That pretty much refactored the whole data structure of the template. Also removed most of the markup in favour of extra fields.
2. #2349615: Copy aggregator templates to Classy (2 months ago) Removed attributes from the html in a classy theme act.

This whole issue needs an re-evaluation on if there is work needed to be done when compared to current situation? So current patch is not really valid anymore.

• Do we modify the html as planned, basically changing top level div to article?
• Do we still want to add the posted_time or just go with the extra fields as one of the issues have stated?

I think we could still add the time and change the main element to article

This patch only contains the div -> article change. How about classy?

And posted_time is not needed right now, as there already is "Posted on" field which uses created timestamp.

classy is done in a seperate issue to not make epic patches - keep em small & clean :)

Here is a patch with classy changes. So if we don't want to fix the classy in this issue, I can create patch into separate issue.

Lets set the issue for "Needs review" so the testbot triggers

We could update the issue summary and add the beta evaluation table.

Issue summary is now updated with beta evaluation table.

cool lets use the right wrappers +1 for article

The template has moved.

Also shouldn't we also change the template in the aggregator module?

Updated tha patch to both fix in core & in class
made sure we didnt forget the {{ attributes }} in the wrapper

Tested the patch. It does what it's supposed to do.

Before

After

I'm not completely sure about the coding standards on twig, but it looks like there's few extra empty lines on rows 25 and 27. Didn't find similar scenario from documentation, but didn't find empty lines either.

It's not something we are explicit about in the standards @see https://www.drupal.org/node/1823416 though by convention we usually don't put those in. They could be removed or not, not too much of a concern. Would be more of a concern if it was double new lines or not 2 spaces.

Thanks for the manual testing @DickJohnson

when i read the twig documentation then we have to remove the white space
so hereby done with an interdiff

Also good:)

Random testbot failure (maybe not random they are deploying a change today)

Templates are not just frozen. Committed b631c16 and pushed to 8.0.x. Thanks!