Convert aggregator-feed-source.html.twig to HTML5

Yay! my first original patch! be kind ...

As hinted above, I based this patch on the aside tag. I also included the details, summary and time tags.

Wow, that was embarrasing. the patch was empty. Hopefully, this one is not.

Thank you Jeff, I'll correct my patch so that the subsequent discussion has a better patch to discuss.

Here are some answers to your questions and a few questions of my own:

<?php print $last_checked; ?>

It is my assumption that this produces a date since it is what the drupal 7 templated used.

<em><?php print t('Updated:'); ?></em> <?php print last_checked;?></em>

Even though my patch says I edited this line, I really didn't. I'm not sure why it's included. I didn't really know the best semantic element to put on this part of the template so I reverted my changes back to what it was. Guess I didn't do that well enough.

@Jeff is right, a valid datetime is:

2011-06-23 16:27:00

note you can put any date you wish in the text of the time element:
<time datetime="2011-06-23 16:23:00">Any string representing the date / time</time>

pubdate should only be set if this is the first time within a sectioning element,

pubdate="pubdate"

See #1183250: Add a theme_datetime() function to consistently theme dates and datetimes

Should we wait on this patch until the datetime patch is finalized?

Or can I roll up Jeff's thoughts into a patch that includes a placeholder for the print datetime function?

Here's a nearly faithful implementation of the code your provided Jeff. The only difference from what you wrote here is that I didn't use the print $datetime function that you referenced. Instead I put a placeholder for it.

I don't mean to stop discussion just moving the patch along with the discussion.

+++ b/modules/aggregator/aggregator-feed-source.tpl.phpundefined
@@ -19,16 +19,22 @@
+
+  </dl>
+</aside>
\ No newline at end of file

End files with a newline :)

Powered by Dreditor.

ah, thought that was a glitch. I'll fix the patch and resubmit.

I'll remember that for the future.

Finally had time to update this patch. Added a newline at the end of the file

subscribe

I added a new date format to Drupal Core called date_format_machine that prints out an ISO Formatted date string e.g. 'Y-m-d\TH:i:s\Z'. The preprocess function for the template is updated to leverage this new date formatter.

Oops! I hadn't seen #1183250: Add a theme_datetime() function to consistently theme dates and datetimes yet!

Please disregard the patch in #16.

Resetting to Needs Work.

I've got this mostly working locally, just waiting on #1183250: Add a theme_datetime() function to consistently theme dates and datetimes to finish up and get committed.

I'm not sure what happens if I upload a patch that depends on a theme function that doesn't exist yet, but I'm going to try anyway. If you want to look at this patch, you need to apply the one in the above-linked issue as well.

changing status to test patch.

1. \ No newline at end of file aggregator-feed-source.tpl.php

2.

+      'url' => $feed->url, 

whitespace

You mean the whitespace at the end of the line? if so, here ya go:

Rerolled for the new D8 /core directory.

Added the theme_datetime function.

Fixed a tiny bug in the call of theme_image where the parameter path had been changed to uri in a previous core commit.

Hey, thanks for the patch @jessebeach. Tagging this to it gets some love this sprint.

What's next? I've tried reviewing this but I'm not seeing anything "wrong". Happy to help tweak this one if necessary.

Nop it doesn't look ok :).
The template changes probably are ok but we need some css to compensate the changes.

Probably only needed for Bartik

Added the following styles to Bartik:

.feed-details dt,
.feed-details dd {
  display: inline;
  margin: 0;
} 
.feed-details dd:after {
  content: '\A';
  white-space: pre;
}

You need to explain this to me because the dd:after looks very creapy to me.
I never used :after before, did some research and it's not support in Luckily we don't have to support those anymore.

What does '\A' do? And white-space: pre; ?
Are 'Url' and 'Title' in italic now?

This CSS should work in IE8+, Chrome, Safari 4+, Firefox 3+, and Opera 10+.

After the <dt> and <dd> elements are set to inline, a Unicode line break character (\A) is inserted into the dd:after psuedo-element to make <dt> elements begin on a new line.

#26: convert-aggregator-feed-source-1189804-26.patch queued for re-testing.

#26: convert-aggregator-feed-source-1189804-26.patch queued for re-testing.

Annoying when files move. Basically a re-roll.

i'd say strip the patch down to just the modifications to the template file.
the first part of the patch doesn't apply anymore due to that function being completely removed.
the middle part of the patch breaks those variables and they don't print out anymore...

here is a screenshot of the result of the patch

i think it would just be best to convert this patch to modify the tpl file, i will follow up with that shortly and see what we come up with

i think this approach is better much simpler, also modified comments to reflect new source data position within the page

Here's the interdiff between 32 & 34.

Needs a reroll now that aggregator-feed-source.tpl.php is aggregator-feed-source.html.twig. New markup looks nice :)

Working on this one

Modified aggregator-feed-source.html.twig to reflex patch #34

#39 Passed and is ready for review, testing or rework.

Looks good to me!

39: convert-aggregator-feed-source-1189804-39.patch queued for re-testing.

Committed/pushed to 8.x, thanks!

This...

+</div

should be this one:

+</div>

right?

Definitely, if you can provide a patch that would be great!

Whoops. Reverted for now.

Even better, thanks @catch. Tagging novice to fix the improperly closed div and post a new patch and interdiff :)

single byte change to patch

Re-committed and pushed to 8.x. :) Thanks!