Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
I'm using .box in my theme and the .box style is now applied to the boxes, what shouldn't be the case, too. Navbar need to use .navbar-box class as example. Other classes are also potentially conflicting. All classes need to be prefixed with it's module name.
If this happens the navbar looks like:
Proposed resolution
Resolution proposed in #12 is to prepend toolbar
to each of the class names in Toolbar module.
Remaining tasks
None
Related Issues
- #1944572: Remove "ul.menu" dependency to prevent theme clashes
- #1847314: Reduce the dependency on JavaScript for the toolbar to display properly
- #1849078: Replace many toolbar icon files with a single CSS sprite image
- #1963824: Reducing CSS selectors weight for toolbar
This issue is a dependency for:
#1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff_72-to-75.txt | 380 bytes | jessebeach |
#75 | prefix-all-toolbar-classes-1938044-75.patch | 45.07 KB | jessebeach |
#72 | prefix-all-toolbar-classes-rebased-on-1847314-1938044-72.patch | 45.16 KB | jessebeach |
#64 | prefix-all-toolbar-classes-rebased-on-1847314-1938044-64.patch | 45.2 KB | jessebeach |
#64 | interdiff_60-to-64.txt | 46.74 KB | jessebeach |
Comments
Comment #1
hass CreditAttribution: hass commentedEDIT: migrated to issue summary
Comment #2
hass CreditAttribution: hass commentedMoving to core first to get broken D8 fixed.
Comment #3
hass CreditAttribution: hass commented.root
is another potential conflicting classComment #4
hass CreditAttribution: hass commentedPatch to fix navbar
.box
and.root
..menu
is not yet fixed.Comment #5
hass CreditAttribution: hass commentedMissed one
.box
. New D7 patch.Comment #6
hass CreditAttribution: hass commentedD8 patch
Comment #7
hass CreditAttribution: hass commentedFixed 80 letters line limit.
Comment #8
andymartha CreditAttribution: andymartha commentedI can confirm that after applying patch core_toolbar_Do+not+expect+ul.menu+and+.box+in+navbar.patch to a fresh installation of Drupal 8.x-dev by hass in #7, the css classes of ul.menu and box were changed with no ill changes that I can see. Tested in Chrome and Firefox on OSX with Bartik and Seven. See screenshot.
Comment #9
webchickLetting Jesse have a peek at this.
Comment #10
hass CreditAttribution: hass commentedThe ul.menu problem is not yet fixed in this patch as I have no idea how, but this must be fixed, too. theme_menu_tree() suxxx a lot as there is no context and don"t allow to inject other classes as I know. theme_preprocess_menu_tree() is not better.
The patch above only fix the .root and .box issue yet with zero side effects. I tried adding .menu class back to the UL's in my theme, but this has shown a lot of troubles and I just removed it again. We still need to find a way to remove the ul.menu dependency.
Comment #11
hass CreditAttribution: hass commentedI think I've nailed out a workaround for ul.menu in my themes, until this is fixed in the navbar/toolbar module.
theme_menu_tree()
don't give you context what suxxx a lot, butTHEMENAME_menu_tree__MENU()
seems to work. TheMENU
is the machine name of the menu. In below example it's themanagement
menu that is used by navbar. Use this as a workaround only if someone needs it.[template.php]
Comment #12
jessebeach CreditAttribution: jessebeach commented.box
and.root
are just two examples from your particular use case. I had wrongly assumed that a theme would not use a selector like.box {}
because it has no context such as.content .box {}
or.page-wrapper .box {}
. But I was being a little naive. Something like the toolbar needs to be sturdier against stray CSS, even if it makes the code itself more verbose.So it seems we'll need to prepend
toolbar
to every class name in the toolbar structure to prevent unqualified selectors in the themes from applying to it. I had had something like this in the first iterations of the module, but I cleaned it out because it seemed verbose. It seems the verbosity also grants it a necessary uniqueness. Is this something you could add to your patch in this issue?I would rather do that in this issue and open a new issue for the
theme_menu_tree
issue. Bringing this up also helped me figure out where a stray clearfix was coming from, so thank you!Toolbar definitely needs to rely on something other than the
.menu
class since that seems to be too volatile. Or maybe we need to update theme_menu_tree to accept a $variables parameter and pass in attributes through it that could be augmented in a preprocess function. Hass, what do you think about that for a followup issue? That would make it easier I think to alter this code in your themes as well.Comment #13
hass CreditAttribution: hass commentedYes to all. :-) If we remove .menu from the ul's we also solve #1938742: Navbar should override margin in system.css (ul.menu li). I will role a patch with all classes prefixed later and maybe with a MODULE_theme_tree__management() if this works... But yesterday this has not worked in a quick test. Could have been a caching issue too, but I'm not sure.
Comment #14
hass CreditAttribution: hass commentedChanging title. I will open a follow up issue to remove
.menu
class dependency.New patch attached.
Comment #14.0
hass CreditAttribution: hass commenteda
Comment #15
jessebeach CreditAttribution: jessebeach commentedIt looks like some elements weren't updated, specifically
.tab
and the menu items.Comment #16
hass CreditAttribution: hass commentedThat one is closer to complete. But something is not yet ready with the horizontal/vertical classes.
Comment #17
hass CreditAttribution: hass commentedMissed 3 class renames
Comment #18
hass CreditAttribution: hass commentedComment #19
oresh CreditAttribution: oresh commented@hass, sorry, the core code has changed, and your patch doesn't apply.
I couldn't find issue related to that, so started my own.
If you provide this patch for latest drupal core changes, i'll help reviewing it and testing
Comment #20
jessebeach CreditAttribution: jessebeach commentedReroll.
I don't endorse all the changes so far, but we are making good progress, so I rerolled to get it to apply without any changes from #17.
Comment #21
oresh CreditAttribution: oresh commentedI'm sorry, your patch breaks everything.
I did a patch here : http://drupal.org/node/1963824, probably you can use the patch from there and add other changes ( I think i pretty much covered all the CSS in the toolbar module. )
Comment #22
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #23
jason.bell CreditAttribution: jason.bell commentedWorking on issue summary update at Portland sprint…
Comment #23.0
jason.bell CreditAttribution: jason.bell commenteda
Comment #23.1
jason.bell CreditAttribution: jason.bell commentedUpdated issue summary.
Comment #24
hass CreditAttribution: hass commentedOk, I've nailed out the few bugs and removed all hunks. Now it's working properly. Please shoot this in asap.
Comment #26
hass CreditAttribution: hass commented#24: toolbar-class-name-refactoring-1938044.patch queued for re-testing.
Comment #27.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #27.1
hass CreditAttribution: hass commentedRemove outdated info
Comment #28
hass CreditAttribution: hass commented#24: toolbar-class-name-refactoring-1938044.patch queued for re-testing.
Comment #29.0
(not verified) CreditAttribution: commenteda
Comment #29.1
hass CreditAttribution: hass commenteda
Comment #30
hass CreditAttribution: hass commented#24: toolbar-class-name-refactoring-1938044.patch queued for re-testing.
Comment #32
hass CreditAttribution: hass commented#24: toolbar-class-name-refactoring-1938044.patch queued for re-testing.
Comment #34
hass CreditAttribution: hass commentedSuxxx broken robot.
Comment #35
hass CreditAttribution: hass commented#24: toolbar-class-name-refactoring-1938044.patch queued for re-testing.
Comment #37
hass CreditAttribution: hass commentedFollow core changes
Comment #38
hass CreditAttribution: hass commentedMissed a line break
Comment #39
yoroy CreditAttribution: yoroy commentedHow much does this issue overlap with #1944572: Remove "ul.menu" dependency to prevent theme clashes ?
Comment #40
hass CreditAttribution: hass commentedVery much... We need to commit this one here first. The other is a followup and touches a lot the lines changed here, too. I have not understood how to theme the menu in a module. Therefore I'm not a good candidate for the followup without some hints.
Comment #41
hass CreditAttribution: hass commentedNeeds a quick re-role as #1987066: Rename files to match CSS file naming convention has been committed.
Comment #42
jessebeach CreditAttribution: jessebeach commentedadding tags.
Comment #43
hass CreditAttribution: hass commentedThis was a bit more work than only paths. JSLint and some other changes have been committed.
Please review asap.
Comment #44
hass CreditAttribution: hass commented@jesse: Can we get this committed, please?
Comment #45
xmacinfoFrom my point of view, this is RTBC! Any other who can review and make it RTBC?
Comment #46
Wim Leers@hass: can you reroll? It doesn't apply anymore.
Comment #47
xmacinfo#43: toolbar-class-name-refactoring-1938044-43.patch queued for re-testing.
Comment #49
hass CreditAttribution: hass commentedComment #50
hass CreditAttribution: hass commentedCan we get this committed now, please? I do not like to reroll again.
Comment #51
hass CreditAttribution: hass commented#2029187: [meta] Make sure CSS classes are prefixed properly
Comment #52
ry5n CreditAttribution: ry5n commented+1 to namespacing classes. This really should follow the CSS coding standards though. For example, I would refactor this:
to something (tentatively) like this:
Let me say clearly that my intention is not to derail this; applying the CSS standards could be a follow-up, preferably as part of #1921610: [Meta] Architect our CSS, work on which is being done in the Mobile Initiative sandbox: https://drupal.org/project/1488942/.
Comment #53
jessebeach CreditAttribution: jessebeach commented@hass, thank you for the diligent rerolls! I just went through #1847314: Reduce the dependency on JavaScript for the toolbar to display properly today. I'm going to be looking at this issue tomorrow. I want to get this committed before July 1st!
@ry5n, I'm not sure how I would realistically construct this class. Here's my dilemma:
.toolbar-bar
-> added in PHP intoolbar_element_info()
for the toolbar element in the Toolbar module..contextual-toolbar-tab
-> added in PHP inhook_toolbar()
in the Contextual module..toolbar-tab
-> added in PHP intoolbar_pre_render_item()
, invoked by the Contextual module's specific call tohook_toolbar()
.SMACSS is meant for building neat, isolated components. What we're building here are layers of semantic input that all get combined and output on HTML elements. I haven't yet wrapped my head around how we take a CSS architecture approach like SMACSS that assumes modularity at an HTML component level and square that with the reality of Drupal's hightly-alterable HTML rendering model. I'm very honestly just expressing the gap in my understanding. Is this something that the Mobile initiative folks have addressed?
Comment #54
ry5n CreditAttribution: ry5n commentedWell said; to be honest, you’re not alone :) The Mobile folks are working on applying the new approach to core in a sandbox (linked above). Ultimately practicality is what SMACSS is all about, so applying those ideas to Drupal will require some creativity and maybe some unique solutions. Again, I don’t think anything along those lines has to happen here, but I couldn’t help calling attention to the new CSS approach, since it does have a lot of overlap with this issue, and I think it’s still not received a lot of attention.
Comment #55
rteijeiro CreditAttribution: rteijeiro commentedRe-rolling due to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute has been commited.
Comment #56
Dragan Eror CreditAttribution: Dragan Eror commented@rteijeiro are you working on it? If so please assign it to yourself.
Comment #57
Dragan Eror CreditAttribution: Dragan Eror commentedComment #58
Dragan Eror CreditAttribution: Dragan Eror commentedApplying changes related to #2015789
Comment #60
Dragan Eror CreditAttribution: Dragan Eror commentedComment #61
Dragan Eror CreditAttribution: Dragan Eror commentedComment #62
rteijeiro CreditAttribution: rteijeiro commentedSome little changes:
[dir=rtl]
must be[dir="rtl"]
according to #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attributeIn file
core/modules/toolbar/css/toolbar.theme.css
I guess[dir=rtl] .toolbar .vertical > .lining:before
has been wrongly changed to[dir=rtl] .toolbar-toolbar .toolbar-vertical > .toolbar-lining:before
[dir=rtl] .toolbar .toolbar-horizontal .toggle-orientation
must be[dir=rtl] .toolbar .toolbar-horizontal .toolbar-toggle-orientation
.toolbar-icon-user.active:before
must be.toolbar-icon-user.toolbar-active:before
Comment #63
Dragan Eror CreditAttribution: Dragan Eror commentedWill not change
according #2031641: Change active class to is-active
Comment #64
jessebeach CreditAttribution: jessebeach commentedWe don't need to prefix classes like
.active
and.open
. These classes indicate a state, not an element of the toolbar component. No library or framework should be targeting these classes without qualification, unlike.toolbar
,.bar
or.tray
which might be targeted without qualification. By qualification, I mean without a scoping component class like.bootstrap.toolbar
. Plus, as @Dragan Eror notes in #63, these state classes will probably become something like.is-active
, so we want to keep state free of namespacing from any particular module.Let's handle this in #2030925: quote rtl attribute selector. The selectors that won't be addressed in #2030925 because they are new have been updated to include the quotations.
Fixed.
Other than not prefixing the state classes (
.active
and.open
), I've taken #60 and rebased it on #1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly. We need to get that CSS/JS issue reviewed and committed first, then we can commit this prefixing issue. Then we can commit #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, which is next on my list to redo today; I will be basing #1860434 on this issue. It's a chain of dependencies, but it will finally clean up all the toolbar issues that have been lingering.I included an interdiff, but it's a little disingenuous because it includes the changes from #1847314.
Built on:
0329b221e3c812de72c76a9a9205dc53d63ded93
#1847314-24: Reduce the dependency on JavaScript for the toolbar to display properly
Comment #65
jessebeach CreditAttribution: jessebeach commentedgo bot, go.
Comment #67
Wim LeersI'll reroll.
Comment #68
Wim LeersOh, never mind, this is rolled on top of #1847314: Reduce the dependency on JavaScript for the toolbar to display properly, that's why it doesn't apply. So instead, a review!
Review
No noticeable differences while using, as expected.
Code nitpicks:
This is not in sync with the actual filename.
Here you use
find('> .something')
, later in this hunk y ou usechildren('.something')
. We should standardize on one.Comment #68.0
Wim Leersa
Comment #69
hass CreditAttribution: hass commentedThis case is all about prefixing css class names. We should fix this find issue in a followup.
Comment #70
xmacinfo@Wim Leers: Why the postpone?
Comment #71
jessebeach CreditAttribution: jessebeach commentedI'll address the comments in a followup, coming shortly.
@xmacinfo, because this patch is dependent on #1847314: Reduce the dependency on JavaScript for the toolbar to display properly
Comment #72
jessebeach CreditAttribution: jessebeach commentedRebased on #1847314-37: Reduce the dependency on JavaScript for the toolbar to display properly.
Comment #73
jessebeach CreditAttribution: jessebeach commented#1847314: Reduce the dependency on JavaScript for the toolbar to display properly was committed. Setting this issue to needs review.
Comment #74
Gábor HojtsyI only found two unrelated bits in the patch, the comment filename one was already found above by Wim and was not yet fixed.
This does not seem to be related to prefixing?
I'm not sure why the file name in the comment would need changing if the file name stay the same?
Comment #75
jessebeach CreditAttribution: jessebeach commentedCorrect, it's not. It's an RTL styling that was incorrect. The LTR version had changed, but not the RTL. I noticed this while testing the patch.
Greedy regex :) I changed the text in the comment back. Diff is below:
Comment #76
jessebeach CreditAttribution: jessebeach commentedInterdiff for #75.
Comment #77
Gábor HojtsyLooks all good now to me! Its a search and replace of class names + that single outdated RTL style fixed. This also blocks #1860434: Refactor the Toolbar JavaScript to use Backbone; fix several poorly functioning behaviors, so marking as blocker.
Comment #78
markhalliwellCan't wait for this to get in. I've been converting Bootstrap to 8.x and the toolbar is affected because of
.icon
classes, would be nice to have these prefixed.Comment #79
Gábor Hojtsy@catch committed this at http://drupalcode.org/project/drupal.git/commit/788cb06d60155575dda548aa... Yay!
Comment #80
catchCommitted/pushed to 8.x, thanks!
Comment #81
Gábor HojtsyRe-remove tag.
Comment #82
dsnopekHurrah! Thanks to hass, jessebeach, Dragan Eror, Gabor and everone else involved. :-)
Comment #83
hass CreditAttribution: hass commented@jesse: should we move this case now to navbar module to get this one fixed, too?
Comment #84
xmacinfo@hass: I would suggest you to create a new issue.
Comment #85
jessebeach CreditAttribution: jessebeach commented@hass, yes! Now is the time. Took us a little while to get there, but we did :)
Comment #86
nod_tag
Comment #87
hass CreditAttribution: hass commentedWe should also get #1944572: Remove "ul.menu" dependency to prevent theme clashes fixed so other modules and theme functions are not cluttering the toolbar like Bartic does.
Comment #88.0
(not verified) CreditAttribution: commentedadded a dependency