Implement secondary tabs based on the designs

I've applyed the css spacer modification.
.

+++ b/core/themes/claro/css/components/content-header.css
@@ -55,7 +55,7 @@
 .content-header {
...
-  margin-bottom: 2rem;
+  margin-bottom: 1rem;

I'm not sure if we can just change the margin of the content header because of the margin specified in the content header is still 2rem. I assume the margin should be 1rem only when there are tabs.

I'm not sure too... but actually claro doesn't discriminate the margin when there're and there aren't the tabs. The only way to change the tabs space (margin) is to reduce .content-header margin-bottom rule.
I will wait for explainations ...

have a nice day. :)

Agreed that it's not clear. I asked for clarification from @ckrina who is one of the designers and the author of this issue.

@ckrina maybe this could be a nice solution.
I've added a little piece of js code inside the nav-tabs.js library that add the ".with-secondary-tabs" class to the only when really there are the secondary tabs.

Attached the screenshots that show the behaviors.

The code that add the discriminant class is:

// If the current iteration regards the second tabs level then apply a class 
// to the <header> to prevent 2rem margin space.
if (i === 1) {
  var $contentHeader = $('header.content-header');
  $contentHeader.addClass('with-secondary-tabs');
}

.content-header.with-secondary-tabs {
  margin-bottom: 1rem;
}

#11, I have reviewed the patch and its working for me. I am attaching the screenshot after applying the patch.

+1 RTBC
#11 Patch works for me.

I'm not sure if we need JavaScript to get this done given that this isn't really interactive. Let's try to come up with a CSS only solution for this.

I've tried, before the js solution, to handle the issue with pure css solution. But as you can see i've failed!
The other thing that conviced me to adopt a js solution it was the fact that in another place (if i remember right) also was used to resolve a similar issue.

(ps: happy new year)

Hi, have attached a screenshot where the space seems to be good, when we reduce it to 1rem, have raise a patch for the same, do review.

I have created a patch. Below points are implemented in this patch:

• Reduced the margin-bottom of header to 1rem(16px) as per the requirement in figma designs.
• Removed the padding-top of secondary tabs as it was adding the space of 6px in the vertical spacing between primary and secondary menu tabs.

Kindly review the patch and screenshot attached.

Adding interdiff for the patch in comment #20.

@kishor_kolekar can you reroll my patch #18 and then add your code for fix with interdiff, in order to track the changes.

Here I have re-rolled patch #18
Made changes those done in patch #20 and added interdiff.

Unfortunately, this approach doesn't match with the style guide. See #2.

previously added the code in incorrect file , so added the code in correct file and created the patch with the interdiff

#28 applied successfully. The secondary tab size is looking good to me.

Unfortunately, we can't simply just change the margin of the content header because of the margin specified in the content header is still 2rem. The margin should be 1rem only when there are tabs.

here is the simple css solution .
kindly review my patch

Patch from #34 applied cleanly and matches the proposed resolution.

This looks good, passes tests and meets specification. RTBC+1

Thanks all for working on Claro theme!

1. @Adam Szałapski At the DGCW, I mentioned that drupal does not use compilers to generate css files. I was wrong.

Since Drupal 8.8.x we're using PostCSS for compiling creating CSS so we also need to change *.pcss.css files. See Drupal core using PostCSS for development.

2. Margin when there are no secondary tabs. It is a bit problematic.

Unfortunately, we can't simply just change the margin of the content header because of the margin specified in the content header is still 2rem. The margin should be 1rem only when there are tabs.

@lauriii Let's add 2rem space to .page-content. Then we can use a 1rem space to .content-header. What do you think about it?

--- a/core/themes/claro/css/layout/layout.pcss.css
+++ b/core/themes/claro/css/layout/layout.pcss.css
@@ -1,7 +1,8 @@
 /**
- * Add spacing to bottom of pages
+ * Add spacing to bottom and top of pages.
  */
 .page-content {
+  margin-top: 2rem;
   margin-bottom: 80px;
 }

--- a/core/themes/claro/css/components/content-header.css
+++ b/core/themes/claro/css/components/content-header.css
@@ -55,7 +55,7 @@

 .content-header {
   overflow: hidden;
-  margin-bottom: 2rem;
+  margin-bottom: 1rem;
   padding: 1.5rem 0 0;
   background-color: #f3f4f9;
 }

Thanks, Krzysztof Domański for valuable information about PostCSS configuration and few tips to fix spacing issues. Please review my newest patch.

Tested manually. Looks good to me.

The current patch increases margin on the bottom of the tabs on mobile. Is this intentional?

Spacing between tabs is OK

The current patch increases margin on the bottom of the tabs on mobile. Is this intentional?

1. Spaces on mobile have been changed according to the Drupal Design system.

2. Also the space above the "Add content" button is still the same.

Oh right, it needs work.

Tested patch from #47 and result is OK

Thank you all for working on this! This is great progress.

+++ b/core/themes/claro/css/layout/layout.pcss.css
@@ -1,7 +1,9 @@
 .page-content {
+  margin-top: 2rem;

It seems like this should be 1.5rem in mobile.

+++ b/core/themes/claro/css/layout/layout.pcss.css
@@ -3,7 +3,7 @@
-  margin-top: 2rem;
+  margin-top: 1.5rem;

Thank you! We shouldn't remove the 2rem completely because it is the correct value for desktop. We should use media query for allowing this property to take effect on large viewports.

Tested patch from #53 and result is OK

1. +++ b/core/themes/claro/css/components/tabs.pcss.css
   --- a/core/themes/claro/css/layout/layout.css
   +++ b/core/themes/claro/css/layout/layout.css
   
   +++ b/core/themes/claro/css/layout/layout.css
   --- a/core/themes/claro/css/layout/layout.pcss.css
   +++ b/core/themes/claro/css/layout/layout.pcss.css
   

   Let's regenerate the layout.css 😇

2. +++ b/core/themes/claro/css/layout/layout.pcss.css
   @@ -1,10 +1,18 @@
   +@media screen and (max-width: 38em) {
   

   We have generally preferred usage of min-width selectors on media queries.

it looks good to me.

Hi,
The #59 patch is working for me.

There's a regression on the secondary tabs focus effect:

Fixed secondary tabs focus effect.

Tested patch from #64 in Chrome, FF, Safari, IE for mobile and desktop resolutions and result is OK

Discussed with @ckrina about the secondary tabs designs and it seems like there are at least a couple of things that were clarified in the designs that need to be addressed here too:

1. The spacing above the secondary tabs should be 0.75rem (space S)
2. The font-size in secondary tabs should be 0.889rem (font-size S)
3. The min-height of the secondary tabs should be 40px

patch https://www.drupal.org/files/issues/2020-05-07/3097651-71.patch fixes the css problem see the below screenshot

#71 Looks good,addresses concerns from #69.

This looks great! We're currently on a code freeze but I will get back to this once the freeze is over.

I had to run yarn run lint:css --fix and yarn run build:css. Interdiff attached here.

Committed 2d166ac and pushed to 9.1.x, 9.0.x and 8.9.x. Thanks!

Here's the missing interdiff