Problem/Motivation

aggregator-item.html.twig markup contains generic div wrapper that should be replaced with more suitable element.

Proposed resolution

Replace main wrapper element <div> into <article>.

Remaining tasks

  1. Review patch

User interface changes

None

API changes

None

Original report by @jeff-burnz

Potential use of article, header, time (with pubdate) and footer.

http://api.drupal.org/api/drupal/modules--aggregator--aggregator-item.tp...

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because item template contains outdated html semantics
Issue priority Not critical because the issue is only with html semantics
Unfrozen changes Unfrozen because RC is markup freeze, not beta.

Comments

cosmicdreams’s picture

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?

Jeff Burnz’s picture

The second one makes no sense - the header and footer elements (if used) need to be children of the article element, as does the time element - use the first structure.

zachattack’s picture

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?

Jeff Burnz’s picture

The issue is for aggregator-item.tpl.php.

I agree that it could be easy to get carried away with the semantic elements just because we can - we should only use what we need.

Jacine’s picture

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

cluke009’s picture

FileSize
1.63 KB
PASSED: [[SimpleTest]]: [MySQL] 46,230 pass(es). View

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.

cluke009’s picture

Status: Active » Needs review

Testing

cluke009’s picture

FileSize
1.69 KB
PASSED: [[SimpleTest]]: [MySQL] 46,232 pass(es). View

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.

Cottser’s picture

Title: Convert aggregator-item.tpl.php to HTML5 » Convert aggregator-item.html.twig to HTML5
Status: Needs review » Needs work
Issue tags: +Twig

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.

Dragan Eror’s picture

Assigned: Unassigned » Dragan Eror
Issue summary: View changes

Let me check this one...

Dragan Eror’s picture

Assigned: Dragan Eror » Unassigned
Status: Needs work » Needs review
FileSize
1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 59,112 pass(es). View

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

mortendk’s picture

FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,074 pass(es). View

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)

tlattimore’s picture

FileSize
2.65 KB
PASSED: [[SimpleTest]]: [MySQL] 59,087 pass(es). View

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

mortendk’s picture

  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

mortendk’s picture

FileSize
3.45 KB
PASSED: [[SimpleTest]]: [MySQL] 59,013 pass(es). View

the categories data wasnt correct thats fixed now

Jeff Burnz’s picture

  1. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -74,19 +74,18 @@ function template_preprocess_aggregator_item(&$variables) {
    +    $variables['categories'][] = array(
    

    Why the change to a numeric array? I think its easier to work with an associative array, e.g. unset one key etc.

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

    Shouldn't this be {{ attributes.class }} {{ attributes }} etc, can we assume only class attributes?

  3. +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
    @@ -8,36 +8,50 @@
    +        <strong>{{ 'Categories'|t }}: </strong>
    

    h3?

Theres some extra lines at the bottom of template_preprocess_aggregator_item() also.

mortendk’s picture

FileSize
3.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,104 pass(es). View

* 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

rteijeiro’s picture

FileSize
3.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,226 pass(es). View

Removed some whitespaces in previous patch.

joelpittet’s picture

Status: Needs review » Needs work

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.

Dragan Eror’s picture

Assigned: Unassigned » Dragan Eror

Will take over to make these changes...

Dragan Eror’s picture

Assigned: Dragan Eror » Unassigned
Status: Needs work » Needs review
FileSize
3.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,824 pass(es), 0 fail(s), and 34 exception(s). View

Patch rerolled, changes are made...

joelpittet’s picture

@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...

Status: Needs review » Needs work

The last submitted patch, 21: aggregator-item-1189806-21.patch, failed testing.

Dragan Eror’s picture

@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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.47 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,075 pass(es). View

Reroll

joelpittet’s picture

Status: Needs review » Needs work

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:)

lauriii’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch aggregator-item-1189806-27.patch. Unable to apply patch. See the log in the details link for more information. View
710 bytes

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

Status: Needs review » Needs work

The last submitted patch, 27: aggregator-item-1189806-27.patch, failed testing.

jussil’s picture

Assigned: Unassigned » jussil
jussil’s picture

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?
lauriii’s picture

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

jussil’s picture

FileSize
495 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,720 pass(es). View

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.

mortendk’s picture

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

jussil’s picture

FileSize
1.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,227 pass(es). View

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.

lauriii’s picture

Status: Needs work » Needs review

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

lauriii’s picture

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

jussil’s picture

Issue summary: View changes
jussil’s picture

Issue summary is now updated with beta evaluation table.

lauriii’s picture

Issue summary: View changes
jussil’s picture

mortendk’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +frontend

cool lets use the right wrappers +1 for article

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

The template has moved.

alexpott’s picture

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

mortendk’s picture

Status: Needs work » Needs review
Issue tags: +classy
FileSize
1.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,254 pass(es). View

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

DickJohnson’s picture

Assigned: jussil » Unassigned
FileSize
156.6 KB
168.17 KB

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

Before
Before

After
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.

DickJohnson’s picture

Status: Needs review » Needs work
joelpittet’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

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

mortendk’s picture

FileSize
1.14 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,247 pass(es). View
698 bytes

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

mortendk’s picture

Status: Reviewed & tested by the community » Needs review
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Also good:)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: agrregator-item-1189806-47.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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

  • alexpott committed b631c16 on 8.0.x
    Issue #1189806 by mortendk, lauriii, jussil, Dragan Eror, cluke009,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.