Posted by Jeff Burnz on June 15, 2011 at 10:31pm
13 followers
Issue Summary
This one is interesting because its for the printed version of book pages, so needs some thought and research about html5 is supported by browsers for print (especially legacy browsers).
http://api.drupal.org/api/drupal/modules--book--book-export-html.tpl.php/7
Comments
#1
should the effort to assign the book entities microdata be apart of this effort?
http://schema.org/Book
#2
I would keep the structured data (microdata and mappings to schema.org) separate from the purely HTML5 markup. Also, bear in mind that like in Drupal 7, the syntax (RDFa/microdata) should remain distinct from the schema mappings (schema.org or FOAF, or OGP). That's why in D7, the schema is stored in the rdf_mapping table, and the syntax is dealt with in the theme layer with preprocess functions. In other words, you don't want to hard code schema.org references into the theme layer, that's something themers should not have to see or worry about.
#3
changes to doctype and html tag needed. Anything else?
I don't think this DIV below needs to change, as it appears it's only for styling and has no semantic meaning.
<div class="section-<?php print $i; ?>">#4
#5
#6
fixed the patch.
see also #1221724: Convert book-node-export-html.tpl.php to HTML5
#7
The last submitted patch, book-export-html.tpl_.php-html5-1189834-4.patch, failed testing.
#8
I really think we need to wait for html and page templates to finalize before moving on this, or even if we actually even need this template.
So waiting on:
#1077566: Convert html.tpl.php to HTML5
#1077578: Convert page.tpl.php to HTML5
#9
Per Jacine's request, I'm marking that I'd like to help review this patch
#10
Tagging this for the next sprint.
#11
added /core path
#12
The last submitted patch, book-export-html.tpl_.php-html5-1189834-11.patch, failed testing.
#13
+++ b/core/modules/book/book-export-html.tpl.phpundefined@@ -16,8 +16,8 @@
+<html<?php print $html_attributes; ?>>
tests are failing because of $html_attributes...
Where did this come from and do we need it?
27 days to next Drupal core point release.
#14
I think it outputs the RDF stuff - I copied it from html.tpl.php
Should I take it out or should we fix the test?
#15
This should pass (if the suggestion thingie works and I implemented it correct).
But I'm not sure this is all we need for book module...
Needs review to test
#16
The last submitted patch, book-template.patch, failed testing.
#17
This looks like a great improvement, and would fix xml:lang to be lang via $html_attributes. Yes, we need it. Just like #1330922: Drupal 8 HTML 5 page template should use lang not xml:lang.
#18
#19
#20
So I started working on this tonight but wanted to see where the patch was at through manually testing it. I was unable to get anything to come up by looking at the source of what this template is supposed to generate.
I expect to see something, anything. Am I doing it wrong?
EDIT:
In checking my error logs I found this:
Warning: include(/home/blah/drupal/core/modules/book/book-export.tpl.php): failed to open stream: No such file or directory in theme_render_template() (line 1395 of /home/blah/drupal/core/includes/theme.inc).
Warning: include(): Failed opening '/home/blah/drupal/core/modules/book/book-export.tpl.php' for inclusion (include_path='.:/usr/share/php:/usr/share/pear') in theme_render_template() (line 1395 of /home/blah/core/includes/theme.inc).
#21
@cosmicdreams: well, the patch from @aspilicious renames the file, so you should rebuild your theme cache after applying the patch. The error messages posted indicate your system is still looking for the old file.
#22
@Gabor ah thank you I didn't notice that. I'll test again tonight.
#23
Let's try this testbot
#24
#23: 1189822-book-export.patch queued for re-testing.
#25
I think this will fail. ->language is ->langcode now
#26
The last submitted patch, 1189822-book-export.patch, failed testing.
#27
#28
The last submitted patch, 1189822-book-export-27.patch, failed testing.
#29
DAMNIT! I hope this one works :)
#30
The last submitted patch, 1189822-book-export-29.patch, failed testing.
#31
Stupid me. Let's try this one.
#32
#33
Patch achieves the base goal outlined, further cleanups can happen in followups AFAIS.
#34
Committed to 8.x. This language part (not the HML5 part) probably needs a backport to 7.x.
#35
I think the extent of the backportability of this patch is something like this (hand edited diff):
-<html xmlns="http://www.w3.org/1999/xhtml" lang="<?php print $language->langcode; ?>" xml:lang="<?php print $language->langcode; ?>">+<html xmlns="http://www.w3.org/1999/xhtml" lang="<?php print $language->langcode; ?>" xml:lang="<?php print $language->langcode; ?>" dir="<?php print $language->dir; ?>">
(Also $language->dir is prefilled in the page template code to the right ltr/rtl designation, not sure that it actually happens in a preprocess here ATM). In effect, the missing dir value should be added. Otherwise changing the book export template to use $html_attributes would be a feature change for theme writers, no?
#36
(Also, D7 uses $language->language, not $language->langcode notably).
#37
Last (D7) patch of the day!
#38
The last submitted patch, book_export-1189834-d7-37.patch, failed testing.
#39
Got lazy on friday evening and forgot to run my tests locally. Give this a try.
#40
#41
Good to go
#42
Committed and pushed to 7.x. The commit message says "Issue #1189834 by aspilicious, chrispomeroy, Albert Volkman: Add language direction to book-export-html.tpl.php" since that seemed to be the only thing this was about.
#43
Should be off-sprint then.
#44
Automatically closed -- issue fixed for 2 weeks with no activity.