Tabs style update

Updated link to final specs

Figma says "not finished".

Added related ticket of content header with specs

Work in progress...

On /admin/structure/types/manage/article/display page, in mobile, the secondary hamburger menu does not open/close.

Desktop preview:

Mobile preview:

I haven't tested this locally, but from your screenshot I'd say background colours need a bit of work: in desktop, primary tabs shouldn't have background colour, while in mobile the background shouldn't be the same as the header (although it's really close). In mobile we're using the header background in a 40% opacity, but it's over a white background (which makes it lighter than the background itself). So to make this possible I'd suggest adding the white to the .tabs element and the --color-whitesmoke-o-40 to the .tabs__tab itself.

Thanks for working on this @shaal! It's great to see this moving forward :)

Made some more progress on this one. Could use another round of review 🙂

This implements the border radius according to the style guide.

I've changed the active/focus indicator to use the :before pseudo-element instead of the border to avoid ugly corners and to avoid conflict with the focus (in case you want to implement it with the border to preserve the border-radius from the designs). I've prepared a patch because it was easier than to explain it.

The focus styles still need to be work on, I haven't had time for it, sorry.

Also, if we’re moving the tabs to the top depending if it’s active and we rely on styles based on its html position (i.e. :last-child), it won’t work because they may end up at the top. I would suggest to start using classes added with JS if we're going to rely on that.

I’d also suggest to not implement this “moving” behaviour in the Secondary tabs: if you access into a page and the “Option 1” is in the top and when you navigate to the next page it's in the third position it’ll confuse people. It makes sense in the Primary tab because they behave like a collapsible menu, though.

Test module that may help development: https://github.com/zolhorvath/tab

Seems #10 patch is broken. it fully removes menu-local-tasks.html.twig without adding new one

also {{ source('/themes/claro/images/src/hamburger-menu.svg') }} in templates/navigation/menu-local-task.html.twig
broken in case of location theme in themes/contrib/claro

1. applied interdiff from #10
2. Fixed include of hamburger svg

i agree with @ckrina about that order for secondary tabs. also it gives jumping on tab navigation.

1. Added border-radius to common variable.
2. Fixed tabs--primary override on desktop.
3. Added focus state.

According to design tabs--primary margin top should be removed and belongs to heading

Removed margins in tabs--primary.

1. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,184 @@
   +  font-size: 1rem;
   

   = var(--font-size-base)

2. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,184 @@
   +  font-size: 0.889rem;
   

   = var(--font-size-s)

3. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,184 @@
   +  width: 3px;
   

   convert into rem here too ?

4. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,184 @@
   +  border-top-left-radius: 2px; /* LTR */
   

   = var(--base-border-radius)

5. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,184 @@
   +  padding: 0.5rem 1rem;
   

   = var(--space-xs) var(--space-m)

The current implementation is broken if there are more tabs on the page that can fit into the viewport width, see the attached screenshot.

I strongly suggest to keep the behavior of tabs in seven:

1. Either solve that behavior by pure CSS and remove nav-tabs.es6.js and it's compiled file nav-tabs.js, or
2. Use that for managing tabs collapse/expand

(edit):

Patch contains coding standard errors.

FILE: /Users/zoli/projects/claro/claro.theme
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
 15 | ERROR | [x] There must be one blank line after the last USE
    |       |     statement; 2 found;
 21 | ERROR | [x] Expected 1 blank line before function; 2 found
 56 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
 59 | ERROR | [x] Namespaced classes/interfaces/traits should be
    |       |     referenced with use statements
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Added a patch to fix the issues raised in #20 and CS issues at #21

+++ b/css/src/components/tabs.css
@@ -1,354 +1,184 @@
+ width: 3px;
convert into rem here too ?

There is a mixed usage of both rem and px. Shouldn't we follow either one throughout the project for consistency?

@imalabya, I can't remember where, but we agreed that we prefer rem.

I still see px units in the patch, and the main issue described in #21 (unreachable tabs) isn't solved, so NW.

Started to replace px units by rem (following these guidelines : https://www.drupal.org/docs/8/modules/claro/claro-css-coding-standards). About the unreachable tabs, I'm not sure I'll be able to fix this ^^'

Just updated the values to use more variables and more rem units. I don't know what to do to solve the problem of unreachable tabs, sorry :/ What is the intended behaviour by the way ? I couldn't find examples of it in the Figma.
Finally, I also had troubles applying the previous patch because of changes in variables.css since then, so I don't know how to generate the interdiff file...

@Kami Amiga, could you please provide interdiff?

I took a stab at fixing the invisible tabs issue.

As @lauriii suggested, this patch makes the tabs switch to the collapsed small screen mode whenever there's not enough space on screen for them in horizontal mode.

Rerolled the most recent patch.

I’m testing out this patch and while it's looking good on vertical and horizontal tabs as evidenced on a content edit page, it seems to be wreaking havoc with Views.

The biggest issue is that the Views tabs aren’t marked with the .is-horizontal class, so all non-active tabs are set to display: none, the flex-direction is column instead of row, and the active tab flex-grows to 100% width. If you remove the css hiding non active tabs, it looks like this:

What's the best way to approach this with Claro? Do we want to:

1. Modify .views-display-top markup to add the is-horizontal class (and if so, then maybe that request needs to go through core?)
2. Rethink having vertical tab styles as the default, or
3. Just override the styles in Claro's views-ui.css, or
4. Something else I haven't thought of?

(One additional concern that needs to be addressed for views tabs is the bullet before the "Add" tab, but that problem predates this patch so I'm not sure if it is part of this issue or should be handled separately.)

Good catch @BrightBold! I opened #3051605: Create new template for rendering Views UI tabs. in the core queue but we should work around this in Claro before that gets fixed.

Customized the views tabs markup so that they could be themed separately. I also copied all tabs CSS rules from Views UI and rewrote the selectors to work with the new markup.

Unless specified otherwise, anything reported was found in OSX Safari, Firefox, and Chrome + IE11 on Windows 8

1. The following changes are required for it to pass Javascript coding standards:
   

   -(function ($, Drupal) {
   +(($, Drupal) => {
      function init(i, tab) {
        const $tab = $(tab);
        const $target = $tab.find('[data-drupal-nav-tabs-target]');
   @@ -19,8 +19,8 @@
            if ($tab.hasClass('is-horizontal') && !$tab.attr('data-width')) {
              let width = 0;
    
   -          $target.find('.tabs__link').each(function () {
   -            width += $(this).outerWidth();
   +          $target.find('.tabs__link').each((index, element) => {
   +            width += $(element).outerWidth();
              });
              $tab.attr('data-width', width);
            }
   @@ -47,9 +47,9 @@
       */
      Drupal.behaviors.navTabs = {
        attach(context) {
   -      $(context).find('[data-drupal-nav-tabs].is-collapsible').once('nav-tabs').each(function () {
   -        $(this).each(init);
   +      $(context).find('[data-drupal-nav-tabs].is-collapsible').once('nav-tabs').each((index, element) => {
   +        $(element).each(init);
          });
        },
      };
   -}(jQuery, Drupal));
   +})(jQuery, Drupal);

2. Although it was not caught by eslint, there are several additional uses of function() syntax in nav-tabs.es6.js that should be converted to arrow function syntax
3. Found at several pages, but can be seen at /admin/structure/types/manage/article/fields among others. When the primary tabs collapse as a result of the JS detection, only one tab is available, and it's a link to the page currently being visited. The contents of the actual primary tabs are no longer available at this point. The comps have a hamburger icon for opening the contents of the tab which is not present in the current patch.
   
4. Initially found at admin/structure/types/manage/article/fields but is not specific to the page: The contents of primary & secondary tabs are not vertically centered (it looks like this was due to something specific to my local env)
5. In the Figma specs, primary/secondary tabs have transparent backgrounds, regardless of state. Currently, the background is different on hover and when links have the .is-active class (it looks like this was due to something specific to my local env)
6. Found at /admin/content: the stripe of color indicating the active tab is not flush with the bottom of its container. It looks like this is due to bottom padding on .tabs a which limits the height of the stripe provided by its :before selector.
   (it looks like this was due to something specific to my local env)
   
   This also cuts off the background color of active tabs on the right side, but that should not be an issue once we've taken care the background color behavior mentioned in the previous item in this list.

I'm still in the process of reviewing this, so an additional comment with more items may appear shortly. I wanted to provide the above information sooner than later so others have an opportunity to work on these items while I'm nit-finding in multiple browsers.

IE11 Findings

1. I ran into this issue when visiting /tabs, provided by the helper module mentioned earlier in this issue. The active secondary tab does not seem retain its width, and the tabs do not collapse despite exceeding the viewport width
2. In high contrast mode there is nothing to visually distinguish active tabs or hover state.
3. May be out of scope, but mentioning just in case: In views, go to a new tab then return to the prior page using the back button. The tab of the page you just left is still in focus. This does not happen in other browsers.

Switching to NW for the issues identified in #40 and #41
A review of the code itself is still needed (40-41's issues were discovered from cross browser issues), but that may be simpler to do if the above issues are addressed.

Working on this

I couldn't reproduce #41.3 but other than that this should address all feedback.

Opened follow-up for a pretty significant accessibility issue on the current mobile styles: #3053457: Flex order breaks tabbing order on tabs . I think this can be fixed in a follow-up because mobile tabs keyboard navigation in Seven seems to be pretty broken as well.

Added styles for tabs that are both focused and hovered.

I'll look further into #41.3 and open a followup if needed, but no need to do anything more here as it's out of scope for this issue.

1. +++ b/claro.theme
   @@ -51,13 +52,35 @@ function claro_preprocess_menu_local_tasks(&$variables) {
   +  $link_class_index = array_search('tabs__link', $variables['link']['#options']['attributes']['class']);
   +  unset($variables['link']['#options']['attributes']['class'][$link_class_index]);
   

   This is arguably very unlikely to occur, but if array_search returns false, the unset() call that follows it will result in an error.

2. +++ b/claro.theme
   @@ -310,6 +333,18 @@ function claro_form_media_form_alter(&$form, FormStateInterface $form_state) {
   +  $form['displays']['top']['tabs']['#prefix'] = str_replace('class="tabs secondary"', 'class="views-tabs views-tabs--secondary"', $form['displays']['top']['tabs']['#prefix']);
   

   This is expecting the exact string class="tabs secondary" which could potentially be changed by a contrib module. Perhaps this could use something like Symfony DomCrawler to alter the class attribute directly?

3. JS Errors can be addressed with
   

   @@ -19,7 +19,7 @@
            if ($tab.hasClass('is-horizontal') && !$tab.attr('data-width')) {
              let width = 0;
    
   -          $target.find('.tabs__link').each((i, value) => {
   +          $target.find('.tabs__link').each((index, value) => {
                width += $(value).outerWidth();
              });
              $tab.attr('data-width', width);
   @@ -47,7 +47,7 @@
       */
      Drupal.behaviors.navTabs = {
        attach(context) {
   -      $(context).find('[data-drupal-nav-tabs].is-collapsible').once('nav-tabs').each((i, value)  => {
   +      $(context).find('[data-drupal-nav-tabs].is-collapsible').once('nav-tabs').each((i, value) => {
            $(value).each(init);
          });
        },
   

4. When tabs are collapsed, the burger icon has no hover behavior, and the link (both is-active and the links appearing below it) hover behavior is color-only, but an additional visual change is needed for accessibiility. I suspect this is because this was originally indended for mobile, but the unreachable tabs fix means collapsed tabs are far more likely to appear on desktop browsers (especially if they're zooming)
5. In RTL, the active tab has a little bit of unwanted right border
6. Also in RTL, if the rightmost tab is active and receives focus, the right side of the focus outline is not visible

7. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,229 @@
   +  text-overflow: ellipsis;
   

   For text-overflow to work, this element would need to be set to overflow: hidden;, but in this case it still won't work as the text is all enclosed in an anchor tag set to display: flex; and text-overflow won't apply to elements displayed as flex.

8. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,229 @@
   +  flex-grow: 1;
   

   Is this rule necessary, since this will be the only child element of .tabs__tab?

9. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,229 @@
    }
   

   This rule can be removed, the design has the primary and secondary tabs set to the same font size, so the styles applied to .tabs__tab are sufficient.

10. +++ b/css/src/components/tabs.css
    @@ -1,354 +1,229 @@
    +  border-left: 3px solid var(--color-absolutezero);
    

    /* LTR */

11. If you click into a view by clicking "edit" via admin/structure/views, the active tab (in this case edit) is not highlighted because the .is-active class is not added to the link with the .tabs__link class. Note that if you click the edit tab, the page reloads and the .is-active class is added

12. +++ b/css/src/components/views-ui.css
    @@ -163,46 +163,183 @@ details.fieldset-no-legend {
    +.views-tabs {
    

    If there's a way to do it clearly, it could be helpful to add a @todo referencing https://www.drupal.org/node/3051605, as many of these rules may be unnecessary or will change after that issue is completed.

13. +++ b/js/nav-tabs.es6.js
    @@ -5,29 +5,35 @@
    +    const handleResize = () => {
    +      toggleCollapsed();
    +    };
    

    Is this necessary since its only calling another function? (I'm guessing this had additional logic at some point)

14. +++ b/templates/block/block--local-tasks-block.html.twig
    @@ -0,0 +1,12 @@
    diff --git a/templates/navigation/menu-local-task--views-ui.html.twig b/templates/navigation/menu-local-task--views-ui.html.twig
    
    diff --git a/templates/navigation/menu-local-task--views-ui.html.twig b/templates/navigation/menu-local-task--views-ui.html.twig
    new file mode 100644
    
    new file mode 100644
    index 0000000..b855981
    
    index 0000000..b855981
    --- /dev/null
    
    --- /dev/null
    +++ b/templates/navigation/menu-local-task--views-ui.html.twig
    
    +++ b/templates/navigation/menu-local-task--views-ui.html.twig
    +++ b/templates/navigation/menu-local-task--views-ui.html.twig
    @@ -0,0 +1,17 @@
    

    Should this get a @todo to https://www.drupal.org/node/3051605 as it may be impacted/unnecessary once that is completed?

The related issue https://www.drupal.org/project/claro/issues/3053457
depends on the patch in #45.

Based on the conversation in Drupal slack the problem with tabbing order has to do with the element source order but the active tab on mobile is visually set with flexbox.

This new patch adds in a method for changing the source order when the tab menu is collapsed and restoring its order for the horizontal display.

Tested on Chrome.

Thank you @zrpnr! I closed the follow-up.

#46.8 This is needed for the mobile styles where the link and toggle are placed side by side.

Besides that, all feedback from #46 has been addressed.

Reviewing

1. 

   Instead of hard-coding a specific height we should define a min-height.

2. 

   Active tab border (narrow screens) does not follow design.

3. 

   The almost-invisible-background defined by the design is missing.

4. +++ b/css/src/components/views-ui.css
   @@ -161,48 +161,189 @@ details.fieldset-no-legend {
   +  margin-left: 200px
   

   Missing trailing semicolon.

Suggested narrow solution:

Instead of flexing the active tab and the toggler I suggest positioning the toggler as absolute (top, right, bottom: 0) and placeholding it's area with a right margin applied on the active tab.

It seems like removing the white-space: nowrap has broken the JavaScript that switches between desktop and mobile layouts. We probably should add that back so we don't have to worry about issues related to tab height.

I asked for feedback from the design team regarding the background. The explanation is conflicting. The demo contains a white layer between the header and the tabs. This isn't described anywhere so I'm not sure if it was done on a purpose.

I also noticed a problem with secondary tabs. It seems like the secondary tabs are not switched to the mobile layout on a wide screen even if the tabs overflow the screen.

I think I addressed @huzooka's feedback in #51, made the tabs stretch height, I kept flexbox instead of absolute positioning but the trigger also stretches now too like your suggested solution looks.

1. Changed to min-height to allow for text wrap
2. moved the active tab blue to the li so that it overlaps like in the figma comp, I made some other small adjustments to hover, focus and active styles.
3. Depending on what the design team says @laurii we can change this- but for now I made a new color that is the "visual" gray from the design, just a hex value of #fafbfd
4. I didn't see this in css/src/components/views-ui.css

The secondary tabs were missing a class and a data attr, they should now collapse based on their parent width.

After a conversation with @lauriii and important accessibility suggestions by @andrewmacpherson, I've changed a bit the designs for tabs. Specifically the hover, focus, active+focus, hover+focus styles.

I've also added an example of how a tab with long text should behave on mobile tabs: instead of hiding text, it should jump into a second line.

Rerolled the most recent patch

This implements the design changes. There's one difference in the current implementation compared to the designs, which is that the grey border is extended to the left so that the grey focus ring doesn't look like it's floating.

This is how the design looks like without this change:

And this is what we ended up with:

Cristina approved this in Slack so this is ready for a review 🙂

This addresses the redesign of the tabs very well, I am a little confused by the figma design which shows some overlap in the hover state.
I think this looks cleaner as you've done it, and if Cristina already approved too then it should be good.

I noticed a few details:

1. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,239 @@
   +  color: var(--color-text);
   

   In figma the focused mobile state has --color-davysgrey #222330, but `.is-horizontal .tabs__link` should be --color-text

2. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,239 @@
   +  color: var(--color-text);
   

   in Figma this is #0036B1, absolutezero-hover

3. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,239 @@
   +.tabs__trigger {
   

   The svg is not quite centered vertically, could add:
   ```css
   display:flex;
   align-items:center;
   justify-content:center;
   ```

4. +++ b/css/src/components/tabs.css
   @@ -1,354 +1,239 @@
   +    padding-left: 2rem ;
   

   space before semicolon

+++ b/css/src/components/tabs.css
@@ -1,354 +1,239 @@
+  .is-horizontal .tabs__link {

non focus horizontal tabs__link also need top border-radius

I have attached a patch with these changes already applied, everything else looked ready to be RTBC to me.

There was a mistake in the way the negative margins were calculated for secondary tabs. Attached fix for this. For me this looks good and I think this is RTBC based on #59.

@huzooka pointed out a missing semicolon on Slack. Uploading a new patch to fix that.

@huzooka pointed out also that there's some debugging code in the .theme file. Removed that from the patch.

In review.

1. IE11: menu looks weird with canvas size 990px on Progress test pages (node-tabs video attached)
2. IE11, canvas size 990px, tabs of Progress test pages are too high (see the video)
3. IE11 focuses not only the toggler button, but the svg inside it as well (see the video)
4. Edge: Toggler icon misaligned initially (looks well after focused/clicked)

Potential fix of focusable SVG: https://github.com/PolymerElements/iron-iconset-svg/issues/46#issuecomme...

I'm not sure what #64.1 is referring to. Coudl you post more specific instructions to reproduce the problem?

This patch fixes #64.2-4.

Steps to reproduce issue #64.1:

• Install Drupal 8.7 with Standard profile and set Claro as admin theme
• Make sure that Field UI and Config translation are enabled
• Resize browser window to make the viewport less than 900 pixels (to make our tabs JS active)
• Log in as admin and visit /admin/structure/types/manage/article with IE11.

You should see this:

But now you will have this:

I think this is solved by #66 as well.

In review...

RTBC!

Thank you everyone, this is a huge step forward! 🚀