Rewrite vertical-tabs component inline with our CSS standards

There is some dependency to elements like summary or strong which would be nice to avoid so that these elements could be for example replaced by another element and not getting the layout broken but that would require some HTML modifications to the vertical tabs

Yes I like the structure and the naming is much more clear. The fact that right now the list item is called "vertical-tab-button" does not make any sense at all.

Regarding the anchor if it really differs from a general anchor than it would need a class (which could actually be the .vertical-tab__menu-item__button) Looks a bit overdoing it but really its something that can help in case for example some theme wants to extend seven.

Not sure if that is something that for some reason might be more complicated. I will look a bit into this code to get a more clear perception of it.

Hello,

Making a patch of this issue. I tested in all themes (classy, bartik and seven).I actually made the changes to the following files :

core/misc/vertical-tabs.js
core/themes/seven/css/components/vertical-tabs.css
core/misc/vertical-tabs.css
core/themes/bartik/css/components/vertical-tabs.component.css

I actually had to maintain ul.vertical-tabs__menu in the Bartik override for some reason as stated in the comment that was already there. It needs to override .region-content ul

I added the following naming as suggested :

.vertical-tabs 
.vertical-tabs__menu 
.vertical-tab__menu-item
.vertical-tab__menu-item a /* Kind of hope we can kind a way to get rid of this */
.vertical-tab__menu-item.is-selected .vertical-tab__menu-item-title
.vertical-tab__menu-item-summary 
.vertical-tabs__pane 

And added also these :

.vertical-tab__menu-item-title /* To replace the strong rule */
.vertical-tabs__panes /* For the panes wrapper */

And changed changed some modifiers.

I am attaching some screens, please review. Also I am not sure about the naming of the patch file I put the project name as "drupalcore".

Hello,

Just making a minor fix to the latest patch there where some "strong" elements that where still not using the proper class name.

Please take a look at line 25 of the js script. I changed this class there but I am not sure about it.

I tested and it seemed okay, also without JS (the default "detail" elements rendered okay).

for some reason in Bartik theme I cant remove the element from the selector or it gets overriden.

I will try again and provide some screens.

Hello,

I am attaching a patch with these fixes. On the bartik theme I had to put the following selector :

.vertical-tabs .vertical-tabs__menu

otherwise it would be overriden with the .region-content ul selector and misplace the tabs adding a different margin.

I am also attaching some images.

Also by removing the

text-decoration: none;

from the :hover effect it now has a underline effect on hover but I guess that is also common behaviour?

@joaogarin Thanks for working on this; it is looking much better already!

1. I am attaching a patch with these fixes. On the bartik theme I had to put the following selector :

   .vertical-tabs .vertical-tabs__menu

   otherwise it would be overriden with the .region-content ul selector and misplace the tabs adding a different margin.

   Yes, this is fine. The selector already has a comment mentioning the selector is overqualified:
   /* This is required to win specificity over [dir="rtl"] .region-content ul */

2. Also by removing the

   text-decoration: none;

   from the :hover effect it now has a underline effect on hover but I guess that is also common behaviour?

   The proposed resolution for this patch is to leave the design as is, but only update the CSS in line with our coding standards. The text-decoration is also currently in line with the style guide for Seven, so let's leave it as is currently in HEAD.

Otherwise, this seems ready to go :)

I added the 'text-decoration: none;' line.

Feedback in #11 has been taken care of, I think this can go to RTBC.

1. +++ b/core/misc/vertical-tabs.js
   @@ -21,8 +21,8 @@
   -        var $this = $(this).addClass('vertical-tabs-panes');
   ...
   +        var $this = $(this).addClass('vertical-tabs__panes');
   

   Shouldn't this class be prefixed by js- because it is javascript only. Also is actually needed can't we just use [data-vertical-tabs-panes]

2. +++ b/core/misc/vertical-tabs.js
   @@ -48,7 +48,7 @@
   -            .addClass('vertical-tabs-pane')
   +            .addClass('vertical-tabs__pane')
   

   Also this appears to be javascript only. Isn't all of vertical tabs JS only? Perhaps we should be thinking about #2431671: [meta] Add in js- prefixed classes for separation of JS & CSS functionality

Yes the vertical tabs are really js only so I guess adding a js prefix is not a bad idea at all!

I will try to have a look into this today or tomorrow and get a patch here.

;) good.

So can you can check the patch to see if there is something missing? I will take another look as well to see if something comes up

Had a look into #14. Look good to me

Investigating this @ Drupal South.

Patch #14 applied without error. Now I must study the new CSS coding standards in depth .

Hello,

I applied the patch just two days ago and It was okay. I will give a crack at it today as well to see if its okay.

My brother is in @ Drupal South nickname kyuubi! Have fun =)

resubmitting #10 with that extra one line of text-decoration: none;

This issue is looked resolved, changing status from needs work to needs review.

Adding SprintWeekend2015 tag.

Back to RTBC.

Manually tested - looks like nothing has changed which is great. However considering we are renaming all the classes here I think we should be prefixing them all with js- as per the standards.

@LewisNyman - ah good - thanks for letting me know that - makes complete sense to me. I think other issues have been attempting to add js- to classes incorrectly then. I'll be on the look out. Given that all the browsers we support also support data attributes should we ever see any js- classes?

Needs a reroll.

Rerolled the patch.

Again

CSS and JS is not frozen in beta. Committed e73ce6e and pushed to 8.0.x. Thanks!

+++ b/core/themes/seven/css/components/vertical-tabs.css
@@ -50,60 +49,59 @@
-.vertical-tab-button.selected a,
-.vertical-tab-button a:hover,
-.vertical-tab-button a:focus {
+.vertical-tabs__menu-item.is-selected a,
+.vertical-tabs__menu-item a:hover,
+.vertical-tabs__menu-item a:focus {
   background: #fcfcfa;
   text-shadow: none;
-  text-decoration: none;
 }

This change is unintended (see #11.2) and introduced a regression where vertical tabs text is underlined in the hover/focus/selected state. I have opened a followup at #2459607: Seven vertical tabs have underlined links in hover/focus state.