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
Comment | File | Size | Author |
---|---|---|---|
#11 | refactor-book-module-css-2405445-11.patch | 4.27 KB | crazyrohila |
#8 | interdiff-6-8.txt | 2.86 KB | crazyrohila |
#8 | refactor-book-module-css-2405445-8.patch | 4.27 KB | crazyrohila |
#7 | Child_page___s66e87dd2a810783_s3_simplytest_me.png | 793.59 KB | LewisNyman |
#6 | interdiff-2-6.txt | 3.43 KB | crazyrohila |
Comments
Comment #1
LewisNymanComment #2
crazyrohila CreditAttribution: crazyrohila commentedwe 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.
Comment #3
crazyrohila CreditAttribution: crazyrohila commentedThese 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?Comment #4
LewisNyman@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.
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?
Maybe we can remove the book-navigation class and just have book pager?
Comment #5
LewisNymanI think this might be a good opportunity to introduce a reuseable class to achieve this called js-hidden.
Comment #6
crazyrohila CreditAttribution: crazyrohila commented@lewis thanks for review.
1.
.menu
is being used for child book page list for a page.2.
.region-content ul
andul.menu
are overriding style when using.book-pager
only3. There was class in
system.module.css
called.js-hide
I have used that one here.Comment #7
LewisNymanNice! 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.
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:
We need to update this comment to: " the js-hide class hides this button when Javascript is enabled"
Comment #8
crazyrohila CreditAttribution: crazyrohila commented1. 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.Comment #9
crazyrohila CreditAttribution: crazyrohila commentedComment #10
LewisNymanWe can still remove classes before we hit release candidate 1.
Ah almost! We need a full stop at the end of this comment :( Then we are done!
Comment #11
crazyrohila CreditAttribution: crazyrohila commented@lewis thanks for pointing.
Updated.
Comment #12
LewisNymanGreat, thanks!
Comment #13
alexpottNice tidy up. Markup changes are not frozen in beta. Committed d246439 and pushed to 8.0.x. Thanks!