Follow-up to #2398455: Clean up "book" component in Bartik

Problem/Motivation

The book module CSS is not inline with our standards. The selectors need to be rethought as components and the properties could probably do with a clean up.

Proposed resolution

To remove these problems, we should follow the best practices guidelines outlined on this drupal.org documentation page: http://drupal.org/node/1887918#best-practices

Remaining tasks

Discuss the naming of the components
Implement the component structure
Clean up CSS properties if required

User interface changes

None. The design objects that style the Drupal module's functionality should be look roughly the same after as they do now.

API changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

FileSize
164 bytes
684 bytes
crazyrohila’s picture

we have following components:

book.theme.css

- navigation
- pager

book.admin.css

- js dependent stylei

Created two component files in css folder, and not sure about *.admin.css. attached initial patch.

crazyrohila’s picture

These classes .book-pager are in classy templates. This is applicable to module only. So should we remove all of this css from module and put them in classy theme?

LewisNyman’s picture

Status: Active » Needs work

@crazyrohila Thanks for the patch, we actually have different file naming standards with modules than we do with themes, see the coding standards docs. I think we are better off grouping them under book.theme.css for now. It also looks possible to reduce the component to just one, we could probably clean up the template a little and remove unused classes.

  1. +++ b/core/modules/book/css/components/navigation.css
    @@ -0,0 +1,9 @@
    +.book-navigation .menu {
    

    Is this styling used? I don't see a menu class but I do see the book-pager class, maybe we can just use that?

  2. +++ b/core/modules/book/css/components/pager.css
    @@ -0,0 +1,36 @@
    +.book-navigation .book-pager {
    

    Maybe we can remove the book-navigation class and just have book pager?

LewisNyman’s picture

.js .book-outline-form .form-submit {
  display: none;
}

I think this might be a good opportunity to introduce a reuseable class to achieve this called js-hidden.

crazyrohila’s picture

Status: Needs work » Needs review
FileSize
194.52 KB
3.21 KB
3.43 KB

@lewis thanks for review.

1. .menu is being used for child book page list for a page.
book-menu class
2. .region-content ul and ul.menu are overriding style when using .book-pager only
3. There was class in system.module.css called .js-hide I have used that one here.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
793.59 KB

Nice! I tested the patch to make sure the styling still applies and it does. We can tidy up the old classes though, I noticed that they were still printing now they aren't being used.

+++ b/core/modules/book/book.module
@@ -163,6 +163,11 @@ function book_form_node_form_alter(&$form, FormStateInterface $form_state, $form
diff --git a/core/modules/book/css/book.admin.css b/core/modules/book/css/book.admin.css

diff --git a/core/modules/book/css/book.admin.css b/core/modules/book/css/book.admin.css
deleted file mode 100644

We've deleted this file but we also need to remove reference to this file so it's not being loaded, we can remove the admin library in book.libraries.yml and also remove reference to that library:

'#attached' => [
        'library' => [
          'book/admin',
        ],
      ],
    // Since the "Book" dropdown can't trigger a form submission when
    // JavaScript is disabled, add a submit button to do that. book.admin.css hides
    // this button when JavaScript is enabled.

We need to update this comment to: " the js-hide class hides this button when Javascript is enabled"

crazyrohila’s picture

1. I thought they can be useful as fallback as if I am not aware of all places where they have been used. Removed Now.
2. Removed admin.css from libraries too.

crazyrohila’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Needs work

I thought they can be useful as fallback as if I am not aware of all places where they have been used. Removed Now.

We can still remove classes before we hit release candidate 1.

+++ b/core/modules/book/book.module
@@ -150,19 +150,12 @@ function book_form_node_form_alter(&$form, FormStateInterface $form_state, $form
+    // The "js-hide" class hides submit button when Javascript is enabled

Ah almost! We need a full stop at the end of this comment :( Then we are done!

crazyrohila’s picture

Status: Needs work » Needs review
FileSize
4.27 KB

@lewis thanks for pointing.
Updated.

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Great, thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice tidy up. Markup changes are not frozen in beta. Committed d246439 and pushed to 8.0.x. Thanks!

  • alexpott committed d246439 on 8.0.x
    Issue #2405445 by crazyrohila: Refactor book module CSS files inline...

Status: Fixed » Closed (fixed)

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