(First, verify that the preprocess changes have been made. #2322163: [meta] Consensus Banana Phase 1, move CSS classes from preprocess to twig templates.)

  1. Copy the Twig templates from the core module's templates directory to Classy's templates directory. Include all templates, even ones without classes.
  2. Remove all classes from the core module's template. Remove all classes added with addClass and ones that are hard-coded in the template.
  • If there are classes that are required for basic functionality, discuss whether they should be kept.
  • If there is CSS from the module, or anywhere else, referring to the class, discuss removing it or moving it to Bartik&Seven. Do not move the CSS to Classy.

Twig Templates to Copy

core/modules/book/templates/book-all-books-block.html.twig
core/modules/book/templates/book-export-html.html.twig
core/modules/book/templates/book-navigation.html.twig
core/modules/book/templates/book-node-export-html.html.twig
core/modules/book/templates/book-tree.html.twig

CommentFileSizeAuthor
#18 Screenshot-book-js-2014-11-12.png61.01 KBbradwade
#16 copy_book_templates_to-2349633-16.patch9.89 KBrunand
#8 copy_book_templates_to-2349633-8.patch5.16 KBmortendk
#4 book-to-classy-2349633-4.patch9.93 KBemma.maria
#2 classy-book.diff4.69 KBmortendk
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented
Issue summary: View changes
mortendk’s picture

mortendk CreditAttribution: mortendk commented
Assigned: Unassigned » mortendk
Status: Active » Needs review
FileSize
classy-book.diff4.69 KB

moved the classes out of core
Discussion of the classes - should be done as a follow up, so bikesheedding wont stop classy ;)

emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: mortendk » emma.maria
Status: Needs review » Needs work
emma.maria’s picture

emma.maria
she/her
CreditAttribution: emma.maria commented
Assigned: emma.maria » Unassigned
Status: Needs work » Needs review
FileSize
book-to-classy-2349633-4.patch9.93 KB

The module template folder was added directly to Classy root folder, added them to classy/templates/book/*

Status: Needs review » Needs work

The last submitted patch, 4: book-to-classy-2349633-4.patch, failed testing.

The last submitted patch, 2: classy-book.diff, failed testing.

davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

The templates should go into the root of the templates folder, not a subfolder.

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue tags: -banana +banana cssbanana
FileSize
copy_book_templates_to-2349633-8.patch5.16 KB

removing the id="book-navigation-{{ book_id }}" in book-navigation.html.twig gave me a bunch of test errors

cant find where n why but this should pass the test + move to /templates the discussion of where to put stuff is in #2349559: [meta] Discuss the organization of subfolders in Classy

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Status: Needs work » Needs review
Issue tags: -banana cssbanana +banana, +cssbanana

and testbot lets see what you got

mortendk’s picture

mortendk CreditAttribution: mortendk commented
Issue tags: +drupalhagen
runand’s picture

runand CreditAttribution: runand commented
Assigned: Unassigned » runand
runand’s picture

runand CreditAttribution: runand commented
Assigned: runand » Unassigned
Status: Needs review » Reviewed & tested by the community

As mentioned, the id in book navigation is not removed, but everything else looks ok

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Reviewed & tested by the community » Needs work

The templates are not generated to classy. Dont know whats wrong

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs work » Reviewed & tested by the community

Ok, works while using git apply! Yeah!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/classy/templates/book-node-export-html.html.twig
@@ -15,8 +15,8 @@
-<article id="node-{{ node.id }}" class="section-{{ depth }}">
-  <h1 class="book-heading">{{ title }}</h1>
+<article>
+  <h1>{{ title }}</h1>

Why are making these changes to the classy template? The classy template should have these classes no?

runand’s picture

runand CreditAttribution: runand commented
Status: Needs work » Needs review
FileSize
copy_book_templates_to-2349633-16.patch9.89 KB
davidhernandez’s picture

davidhernandez
Primary language English
Location USA
CreditAttribution: davidhernandez commented

Please double-check if any removed classes are being used in javascript. It is best to test the affected template using Stark to make sure nothing is broken.

bradwade’s picture

bradwade CreditAttribution: bradwade commented
FileSize
Screenshot-book-js-2014-11-12.png61.01 KB

While book.js does reference classes, specifically .book-outline-form and .book-title-select, neither of these were removed in these template changes. (Those classes are added in by BookManager.php)

I tested and the book.js continues to function properly with patch #16 applied. To test this you must enable Stark as your admin theme and add/edit a book page (book module must be enabled). The JS updates the summary under the heading of the "book outline" vertical tab when a new book/parent is selected. Note: This JS is not being executed in the Seven theme as Seven has removed this functionality of displaying a summary along with the vertical tab heading.

To summarize: Double checked. Javascript not broken.

mortendk’s picture

mortendk CreditAttribution: mortendk commented

@bradwave Ok then we should set it back to RTBC :)

bradwade’s picture

bradwade CreditAttribution: bradwade commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change (theme improvements) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 8de9b67 and pushed to 8.0.x. Thanks!

  • alexpott committed 8de9b67 on 8.0.x 
    Issue #2349633 by mortendk, runand, bradwade, emma.maria: Copy book...
mortendk’s picture

mortendk CreditAttribution: mortendk commented

*high fives all around*

Status: Fixed » Closed (fixed)

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