Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Change active
HTML class to is-active
according new CSS architecture (for Drupal 8).
This change requires change in the functions as well as in all CSS files where active class is styled.
Related issues
Beta phase evaluation
Issue category | Bug/Task/Feature because ... |
---|---|
Issue priority | Major because ... Critical/Not critical because ... |
Unfrozen changes | Unfrozen because it only changes markup and CSS |
Comment | File | Size | Author |
---|---|---|---|
#174 | Screenshot 2015-04-16 16.07.24.jpg | 654.78 KB | LewisNyman |
#173 | change_active_class_to-2031641-173.patch | 49.87 KB | nlisgo |
#173 | interdiff-2031641-164-173.txt | 564 bytes | nlisgo |
#147 | 2031641-interdiff.txt | 1.82 KB | rpayanm |
#147 | 2031641-147.patch | 45.57 KB | rpayanm |
Comments
Comment #1
aburrows CreditAttribution: aburrows commentedComment #2
LewisNymantaggin
Comment #3
aburrows CreditAttribution: aburrows commentedNew 'is-active' class added
Comment #4
aburrows CreditAttribution: aburrows commentedModified comment to suit new class name
Comment #5
Dragan Eror CreditAttribution: Dragan Eror commented@aburrows Thanks for this awesome fast replay and patch :)
This is a nice change to l() function, but this issue requires a little bit more changes in all CSS files in core which contains
.active
related styles and should be checked carefully which means Find & Replace can NOT be used.Comment #6
Dragan Eror CreditAttribution: Dragan Eror commented@aburrows Thanks for this awesome fast replay and patch :)
This is a nice change to l() function, but this issue requires a little bit more changes in all CSS files in core which contains
.active
related styles and should be checked carefully which means Find & Replace can NOT be used.Comment #7
aburrows CreditAttribution: aburrows commentedJust about to submit with all the css changes done
Comment #8
aburrows CreditAttribution: aburrows commentedPatch with all css class name changes.
Comment #10
LewisNymanWhen applying I get the error:
Comment #11
aburrows CreditAttribution: aburrows commentedPatch with additions of menu.inc added
Comment #12
LewisNymanComment #14
aburrows CreditAttribution: aburrows commentedThis should now contain all the amended items
Comment #16
aburrows CreditAttribution: aburrows commentedI need to get the latest HEAD and that should solve it.
Comment #17
aburrows CreditAttribution: aburrows commentedPatch is now working with latest head, I though I had latest HEAD but didn't. Now I do and just tested locally and it applied fine.
Comment #19
aburrows CreditAttribution: aburrows commentedThis hopefully should stop the failing tests, hopefully in time before core updates
Comment #21
PolComment #23
PolComment #24
PolOops, my bad.
Comment #26
PolSorry for the noise, I had a problem with my patches!
Comment #27
Dragan Eror CreditAttribution: Dragan Eror commented@aburrows, @Pol I am so happy to see such a contribution from you on this really new task. Already see results from this soon :)
Can you guys please follow the patch naming convention and help core maintainer to not become confused?
Comment #28
aburrows CreditAttribution: aburrows commented@Dragan Eror sorry got into bad practice with that, can't remember where I saw it
Comment #30
aburrows CreditAttribution: aburrows commentedComment #32
RainbowArrayHere's a reroll of this patch.
15 files with merge conflicts! Not surprising after a few months.
What I haven't done is to go through all of Drupal core to see if there are any new files that might have the active class which needs to be changed to is-active. Hope this serves as an updated patch to move forward with that work.
Comment #34
LewisNymanI've found a few more mentions of class 'active' in:
Comment #35
aburrows CreditAttribution: aburrows commented@Lewis, is this issue still outstanding? If so assign to me and i'll sort it tonight.
Comment #36
LewisNyman@aburrows Looks like I missed this. I'm not sure if this should be moved to 9.x?
Comment #37
aburrows CreditAttribution: aburrows commentedYeah I think 9.x would be best because its going to cause to many issues for 8.x
Comment #38
RainbowArrayWe're just changing a name of a class. Yes, there are a lot of places where it's changed, but is it really that much of a change that we need to push this fix out for 3-5 years from now?
Comment #39
Anonymous (not verified) CreditAttribution: Anonymous commentedAfter chatting with Lewis, it seems like this could be a really far-reaching change, even though on the surface it's trivial. Tentatively postponing to 9.x...
Comment #40
LewisNymanThis change will affect every theme. It's a pretty big API change for themers. It feels like more trouble than it's worth.
Comment #41
LewisNymanMorten told me that Alex Pott told him that markup freeze is release candidate 1. So we still have time! We can do this!
Comment #42
LewisNymanComment #43
aburrows CreditAttribution: aburrows commentedHow much time do we have?
Comment #44
Dragan Eror CreditAttribution: Dragan Eror commented@aburrows, release candidate 1 means 4-6 months at least. Currently it's not even beta...
Comment #45
aburrows CreditAttribution: aburrows commentedok cool ill get this done tomorrow when im mentoring, if i have issues ill speak to alexpott about what it will affect :P
Comment #46
aburrows CreditAttribution: aburrows commentedComment #47
LewisNymanLet's get this through in Austin
Comment #48
aburrows CreditAttribution: aburrows commentedHeres the latest patch that changes .active to .is-active
Comment #49
aburrows CreditAttribution: aburrows commentedFixed installation issue for is-active
Comment #50
aburrows CreditAttribution: aburrows commentedComment #51
darol100 CreditAttribution: darol100 commentedI test your patch on SimplyTest.me And the toolbar doesn't work.
Comment #52
aburrows CreditAttribution: aburrows commentedPatch fixed was a JS issue
Comment #53
darol100 CreditAttribution: darol100 commentedThe toolbar have a different problem now. When you click in the toolbar a tab you can't go back to a the first tab. You can tell by the color gray on the toolbar.
Comment #54
aburrows CreditAttribution: aburrows commentedCheers darol100 I've fixed the js issue now please test.
Comment #55
darol100 CreditAttribution: darol100 commentedThe toolbar is working fine. Also, I revise your patch seem to be working fine.
Comment #56
darol100 CreditAttribution: darol100 commentedComment #57
aburrows CreditAttribution: aburrows commentedComment #58
alexpottCan we get a change record before commit
Comment #59
aburrows CreditAttribution: aburrows commentedChange record: https://drupal.org/node/2281785
Comment #60
alexpott/Volumes/devdisk/dev/drupal/core/modules/contextual/js/toolbar/views/VisualView.js
/Volumes/devdisk/dev/drupal/core/modules/tour/js/tour.js
/Volumes/devdisk/dev/drupal/core/themes/seven/js/mobile.install.js
Also can we have some screenshots proving that nothing is borked
Comment #61
lauriiiComment #62
lauriiiHere's a patch for the points that Alex pointed. I have also attached some pictures from the places where active class has been changed to is-active.
Comment #63
aburrows CreditAttribution: aburrows commentedI noticed a few more places missing it, so heres a complete patch with the other places that were missing it
Comment #64
aburrows CreditAttribution: aburrows commentedComment #65
aburrows CreditAttribution: aburrows commentedComment #66
aburrows CreditAttribution: aburrows commentedComment #67
lauriiiRerolled the patch #63
Comment #68
aburrows CreditAttribution: aburrows commentedIs this going to have to be rerolled again? So we can keep up with latest release?
Comment #69
lauriiiComment #70
redsquid CreditAttribution: redsquid commentedComment #71
redsquid CreditAttribution: redsquid commentedRerolled the patch #68. My first
Comment #73
redsquid CreditAttribution: redsquid commentedUps. Wrong branche.
Comment #74
redsquid CreditAttribution: redsquid commentedNot so fast. Third time... maybe
Comment #75
star-szrThanks @redsquid! The patch almost doubled in size from #67 to #74 so I think this needs another look.
Comment #76
tuutti CreditAttribution: tuutti commentedRerolled #67 patch
Comment #77
tuutti CreditAttribution: tuutti commentedComment #78
LewisNymanI manually test this patch and couldn't find any visual regressions in Seven or Bartik. I've attached a few screenshots.
Comment #80
joelpittetComment #81
LewisNymanNeeded reroll mainly because of #2321505: Split Seven's style.css into SMACSS categories
Comment #82
tuutti CreditAttribution: tuutti commentedNeeded reroll again because of #1898478: menu.inc - Convert theme_ functions to Twig
Comment #83
aburrows CreditAttribution: aburrows commentedWhats the status of this, as ill re-roll and re-patch
Comment #84
joelpittet@aburrows it certainly needs a re-roll, could you please?
Also if you do, it would be great to find someone on IRC or at drupalcon to partner up to review your re-roll so this can get RTBC'd
Comment #85
aburrows CreditAttribution: aburrows commentedIt will be completed today
Comment #86
aburrows CreditAttribution: aburrows commentedComment #87
aburrows CreditAttribution: aburrows commentedworking on this now
Comment #88
aburrows CreditAttribution: aburrows commentedComment #89
aburrows CreditAttribution: aburrows commentedComment #90
paulhhowells CreditAttribution: paulhhowells commented* Applied the patch.
* Selected the Admin menu, and
* observed that a class of .is-active was applied, and
* observed that a class of .active was no longer applied:
Comment #91
paulhhowells CreditAttribution: paulhhowells commented* Applied the patch
* Navigated to /update.php
* Observed that a class of .is-active was applied to 'Overview' within the .task-list:
Comment #92
paulhhowells CreditAttribution: paulhhowells commented* Applied the patch
* Navigated to /admin/config
* Observed that a class of .is-active was applied to 'Configuration':
Comment #93
paulhhowells CreditAttribution: paulhhowells commented* Applied the patch
* Navigated to /admin/theme/install
* Observed that a class of .is-active was applied to the 'Update' tab:
Comment #94
paulhhowells CreditAttribution: paulhhowells commentedComment #95
emma.mariaI was the mentor that helped @paulhhowells to review this patch.
- We observed no JS errors that previous reviews above found.
- None of the UI had visually broken parts.
- The console returned no errors.
- The .is-active class is now present in the markup, .active is gone.
I agree with RTBC decision.
Comment #96
Wim LeersRTBC+1, patch looks great!
I'd argue these should be converted to use
data-drupal-link-system-path
, so that theis-active
class can be added by the JS.But, that's of course very much out of scope here. Perhaps that'd be a nice next issue to work on for paulhhowells? :)
Comment #98
lauriiiRerolled
Comment #99
chx CreditAttribution: chx commentedBack to RTBC.
Comment #101
lauriiiRerolled again
Comment #103
lauriiiComment #104
Fabianx CreditAttribution: Fabianx commentedNot that I am opposed to it, but I think those functions sneaked in lazily from another path. I very much assume they did hide themselves in a LazyBeanBagCollection so that you could not see them :-D.
Comment #105
lauriiiRemoved that stuff.
Comment #106
Fabianx CreditAttribution: Fabianx commentedLooks great to me!
Comment #107
alexpottThis conflicts with #2329767: Move table classes from preprocess to templates - the interesting thing is why with an issue titled "Change active class in l() function to is-active" are we changing the active class on the tablesort indicator? This is not the active link.
Comment #108
Wim LeersSorry, just one stupid nitpick:
Please also update the docblock for
Drupal.behaviors.activeLinks
.AFAICT this was manually tested in #90–#95, in which case RTBC+1.
Comment #109
kallehauge CreditAttribution: kallehauge commentedThe code inside the preprocess_table function (#107) doesn't show up in the interdiff since it could not be applied with the previous patch, anymore (#105)
Comment #110
kallehauge CreditAttribution: kallehauge commentedComment #111
lauriiiPatch fixes #107 and #108
Comment #112
alexpottThis is a table cell class not a link class. This issue is about link classes. And it's inconsistent with the current table.html.twig template.
This why this issue has taken so long to get done - because it has not stuck to the scope outlined by the issue title Change active class in l() function to is-active. This has nothing to do with tablesorts.
Comment #113
LewisNymanAh that's right, but we do need to change the active class in the whole of core, not just when added in the l() function. I think when this issue was written the assumption was that it was only appeared in the l() function, but this is not the case.
So we could either:
1. Move the code for the table sort component out of this issue into a follow up.
2. Change the scope and summary of the issue to reflect the current patch, which is in line with the standards.
If we go with two then that means I can set the most recent patch to RTBC, and it saves time and effort.
Comment #114
alexpottThis patch will break tablesort theming then - since
table.twig.html
still uses the active class.Comment #115
alexpottAnd the draft change record needs updating too then.
Comment #116
lauriiiRemoved table related changes. Some other changes because of hunks have changed a bit.
Comment #117
LewisNymanWe shouldn't remove the changes, this needs to be consistent. We should make the change the table.twig.html to fix this instead.
Comment #118
brahmjeet789 CreditAttribution: brahmjeet789 commentedI have changed the class active to is-active from table.twig.html and also some more changes
Comment #119
lauriiiLets move the issues status to Needs review so the testbot triggers. I also added the sprint tag for this issue
Comment #120
LewisNymanIt looks like the most recent patch does not include the changes of the previous patch?
Comment #121
saki007sterMerged the patches in #116 and #118.
Comment #122
saki007sterComment #123
saki007sterAlso tested this the classes have been changed to 'is-active' from 'active'.
Comment #124
akalata CreditAttribution: akalata commentedComment #125
alexpottIs in toolbar.module.css
is in bartik's media.css
is in bartik's primary-menu.css
.active
in bartik's tabs.css.active
in seven's tables.cssThis is just the result of a quick search - we need to ensure we've caught everything here...
Comment #126
alexpottFor example...
in tablesort.inc
in LocalTaskDefault.php
SystemController.php
There a couple of instances of this in views.theme.inc
Comment #127
eldrupalista CreditAttribution: eldrupalista commentedHi, added all the ones listed by @alexpott and a few others.
Comment #128
eldrupalista CreditAttribution: eldrupalista commentedComment #130
lauriiiLets see if the tests pass now
Comment #131
joelpittetJust a quick run through and found a strange one:
This doesn't look like classes, are you sure this is right?
Same here this doesn't look like classes.
This should get a follow-up issue so that it's an array like the rest of core.
Comment #132
lauriiiGood catches, I fixed #131.1-2
Comment #133
joelpittetThanks @lauriii I made a followup for #131.3 #2414413: Make sure we are building CSS classes as arrays
Comment #134
joelpittetBoth of these are 80 char breakers, should be moved to new line.
This looks like it would break JS, and is likely an unrelated change.
This would also break JS, and shouldn't be changed.
Same, this is a breaky change.
This is hilarious, it's converting an array to a string to then explode it back into an array. Could this be fixed here or should we open up a folloup for that too?
Comment #135
RavindraSingh CreditAttribution: RavindraSingh commented@joelpittet, agreed on your 1st point, but I don't think about the other stuff are breaky. as they all are defined and just a replacement of .active class.
Comment #136
RavindraSingh CreditAttribution: RavindraSingh commentedI am retesting it.
Comment #140
idebr CreditAttribution: idebr commentedComment #141
joelpittet@RavindraSingh Read them carefully. '-' character in javascript is a minus sign;-) That's why it's breaky
Comment #142
vineeth@nair CreditAttribution: vineeth@nair commentedComment #143
vineeth@nair CreditAttribution: vineeth@nair commentedComment #144
star-szr#135 looks like it introduces a bunch of unrelated changes (forgot to rebase?), so I'd suggest a reroll should be started from #132 instead.
@vineeth@nair I'm unassigning since it's been a while, if you are in fact working on this still please comment back and feel free to reassign :)
Some of the unneeded changes:
Comment #145
akalata CreditAttribution: akalata commentedUpdating #132 with the feedback in #134.
Comment #147
rpayanmComment #149
nlisgo CreditAttribution: nlisgo commentedComment #150
nlisgo CreditAttribution: nlisgo commentedComment #151
LewisNymanLooks like the last patch included Bartiks media.css file by mistake, which is now removed in HEAD.
Comment #152
nlisgo CreditAttribution: nlisgo commentedGood spot @LewisNyman. Not sure how that found it's way in there.
Comment #153
joelpittetNice work everybody!
Here is a few things as I went line by line through the patch:
Not something to fix in this issue, but that looks like a clobber of other classes on that variable. Needs a follow-up issue to fix that. Should be $variables['attributes']['class'][] = 'is-active'; but we need to check how that effects things in that preprocess.
This looks like a bug fix. Can we move this to another issue or remove it if it's not needed anymore?
This may be scope creep, but I'm ok with it, though we may want to punt it to another issue to keep this from growing, because we may may want also menu-item--active-trail be menu-item--is-active-trail, for consistency.
which was actually done here, lol. Well if there aren't many maybe we can add it to this issue... you be the judge, just keep in mind the bigger the patch, the harder to get reviews, so keep it from creeping too much...
Comment #154
nlisgo CreditAttribution: nlisgo commentedAddressing feedback in #153.
Code in point #2 was removed here #2407745: Remove classes from system templates t*.html.twig, so removing from this patch as requested.
Regarding points #3 and #4, it is clear that at some stage in this issue someone introduced is-active-trail but it was cleared up later on and the css identified in #4 was the only remnant of this work. So, I'm removing and recommend also that this be addressed in a followup issue.
I also removed a blank link which had been introduced to core/themes/bartik/css/colors.css
Comment #156
joelpittet@nlisgo the interdiff and rational looks great, the patch has some extra stuff in it, maybe needs a rebase to catch up to head on that branch you made the diff from? Just a guess.
Thank you for addressing my review points!
Comment #157
nlisgo CreditAttribution: nlisgo commentedRebase done. :)
Comment #158
joelpittetThanks again, trust me I've done that quite a few times too;)
I think this one got missed from being un-done and moved to follow-up?
Comment #159
nlisgo CreditAttribution: nlisgo commentedComment #160
jp.stacey CreditAttribution: jp.stacey at Magnetic Phield commentedReviewed as follows:
* The patch applies cleanly to 300f14e (D8 head as of this morning).
* Testing with a couple of hierarchical links set up in a menu, by clicking between them, .is-active is present on the relevant link, and the link picks up styles accordingly.
Comment #161
LewisNymanThanks, let's get this in before it needs a reroll. I've added the beta evaluation
Comment #162
alexpottWe still have things using the active class.
in core/modules/toolbar/css/toolbar.menu.css
in core/modules/views/templates/views-view-summary-unformatted.html.twig
in core/modules/views/templates/views-view-summary.html.twig
in core/themes/bartik/css/components/primary-menu.css
in core/themes/bartik/css/components/site-footer.css
in core/themes/classy/templates/dataset/table.html.twig
in core/includes/theme.inc
Comment #163
LewisNymanTagging with novice to make these changes
Comment #164
_nolocation CreditAttribution: _nolocation commentedI made the changes from comment #162
Comment #165
rteijeiro CreditAttribution: rteijeiro commentedBack to RTBC!
Comment #166
akalata CreditAttribution: akalata commentedNeeds manual testing and screenshots per alexpott in IRC. Working on those now.
Comment #167
akalata CreditAttribution: akalata commentedVerifying that #164 addresses #162:
New item found:
8. Default template_preprocess_views_view_table(), class depicting active table sort: core/modules/views/views.theme.inc
Comment #168
alexpottComment #169
akalata CreditAttribution: akalata commentedPoint 6 in #167 is likely related to #2364413: The cell.active_table_sort in table.html.twig has no effect.
Comment #170
nod_Comment #171
ti2m CreditAttribution: ti2m commentedI ran the patch through siteeffect and the only thing I found is an issue with table sorting in views on e.g. /admin/content (see attached gif) or /admin/people. This is probably covered through point 8 in #167.
Comment #172
nlisgo CreditAttribution: nlisgo commentedI'm going to take this issue on. First action is to address point 8 in #167 for which a screenshot is provided in #171.
Comment #173
nlisgo CreditAttribution: nlisgo commentedComment #174
LewisNymanThanks for providing the interdiff. I manually tested the patch and I can confirm that it fixes the problem reported by the visual regression testing.
It would be amazing to get this patch committed at DevDays, because it's been open since DevDays in Ireland :D
Comment #175
alexpottLet's do this! Committed b676df9 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #177
aburrows CreditAttribution: aburrows commentedGreat work team!!!!!!!!!!!!
Comment #178
joelpittet<3 state classes!
Comment #180
brahmjeet789 CreditAttribution: brahmjeet789 as a volunteer and at gai Technologies Pvt Ltd commentedComment #181
LewisNyman