Multiple rows of tabs not spacing properly in Seven theme [#1699072]

Comment	File	Size	Author
#33	seven-multiple-user-tabs-1699072-33.patch	358 bytes	manooh

#29	overlay-tabs.png	75.51 KB	idebr
#27	20130922_after.png	48.2 KB	stevenovy
#27	20130922_before.png	47.18 KB	stevenovy
#17	before-multiple-user-tabs-seven.png	19.85 KB	kevinsiji
#17	after-multiple-user-tabs-seven.png	21.02 KB	kevinsiji
#17	seven-multiple-user-tabs-1699072-17.patch	360 bytes	kevinsiji

#12	before-multiple-user-tabs-seven.png	21.18 KB	Shyamala
#12	after-multiple-user-tabs-seven.png	26.56 KB	Shyamala
#12	before_multiple-user-tabs-bartik.png	44.21 KB	Shyamala
#12	after_multiple-user-tabs-bartik.png	44.12 KB	Shyamala
#12	before-multiple-user-tabs-bartik.png	44.21 KB	Shyamala
#12	after-multiple-user-tabs-bartik.png	44.12 KB	Shyamala
#2	changes.jpg	59.55 KB	Vc Developer
#1	tab-wrapping-1699072-1.patch	730 bytes	ishmael-sanchez

Comment #1

ishmael-sanchez CreditAttribution: ishmael-sanchez commented 27 July 2012 at 20:03

Version:	7.14	» 8.x-dev
Status:	Active	» Needs review

File	Size
tab-wrapping-1699072-1.patch	730 bytes

This does seem like an edge case (26 is a lot and you could use hook_menu_alter to modify the tab listing or create a sub-theme and fix), but I think the tabs shouldn't be hidden by presentational CSS.

There is similar issue #1699058: Not handling multiple tabs..... with AdaptiveTheme but the suggestion to

ul.primary {
    white-space: normal;
}

doesn't fix the issue alone you need to remove the exact height definition as well in ul.primary around line 259 style.css.

In Rubik the body page doesn't adjust because in .primary-tabs in the style.css 185 there is a specific height definition so you could adjust that by creating an admin sub-them and make the CSS work for your use case.

Log in or register to post comments

Comment #2

Vc Developer CreditAttribution: Vc Developer commented 28 July 2012 at 15:51

File	Size
changes.jpg	59.55 KB

Yea, that is what I had to do with Rubik, I changed each link to look like buttons using the same style so it wouldn't look funky when it wraps, plus other minor changes.

Take a look at the screen shoot....

Log in or register to post comments

Comment #3

ishmael-sanchez CreditAttribution: ishmael-sanchez commented 28 July 2012 at 16:00

@Vc Developer can you test the patch if you have a chance and if it works at expected mark it as reviewed & tested by the community and hopefully we can get this fix committed so everyone can enjoy the fix.

Log in or register to post comments

Comment #4

Vc Developer CreditAttribution: Vc Developer commented 28 July 2012 at 17:38

Version:

8.x-dev

» 7.14

I need to make a correction, but I will still test it on D8. I didn't realize after trying the patch on D7 that my post somehow was for D8. Do I need to create another issue for D7 to get a patch made?

Log in or register to post comments

Comment #5

ishmael-sanchez CreditAttribution: ishmael-sanchez commented 28 July 2012 at 18:36

Version:

7.14

» 8.x-dev

The problem exists in D8 and D7 so I believe the preferred approach is to fix it in D8 then have the fix trickle down to D7 hence why I marked it D8.

Log in or register to post comments

Comment #6

Vc Developer CreditAttribution: Vc Developer commented 28 July 2012 at 19:04

Ok.., don't know if there's enough modules for D8 to get over lapping tabs, but I will give it a try and get back with you....

Log in or register to post comments

Comment #7

Vc Developer CreditAttribution: Vc Developer commented 28 July 2012 at 19:21

I'm running Cygwin on a Windows Web Server 2008 and I get this message when trying to run the patch:

eqmiller@anointed-media /cygdrive/d/wamp/www/drupal-8.x-dev
$ patch -p0 < tab.patch
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/system/system.theme.css b/core/modules/system/system.theme.css
|index d5f41e1..09c1291 100644
|--- a/core/modules/system/system.theme.css
|+++ b/core/modules/system/system.theme.css
--------------------------
File to patch:

P.S. I shorten the name to reduce typing....

Log in or register to post comments

Comment #8

ishmael-sanchez CreditAttribution: ishmael-sanchez commented 28 July 2012 at 20:01

@Vc Developer try git apply -v [patch-name.patch] (See Making a Drupal patch with Git the Applying a patch section) using that command I was able get the patch to apply cleanly.

For the actual testing of tabs I edited the HTML of the page using firebug and chrome so you can do the same without having to install additional modules to create tabs for you.

Log in or register to post comments

Comment #9

Vc Developer CreditAttribution: Vc Developer commented 29 July 2012 at 00:07

Is this the reason why Drupal code is in such shambles, because of not properly testing with actual live data configuration? This is the average result of all modules being published. Manipulation is not testing, I can only properly test the patch in D7.

I hope this is put an end too in D8.... Its frustrating taking five steps back for every one, because some other module steps all over another and with just one click of the mouse destroys everything..

I'm smiling while I'm saying this so don't take it personal.. ;)

Log in or register to post comments

Comment #10

ishmael-sanchez CreditAttribution: ishmael-sanchez commented 30 July 2012 at 18:17

@Vc Developer

I definitely don't think the Drupal code base is in shambles and if you think it is the best part about open source is you can contribute and make it better. Did I take a shortcut, yeah because this is just a presentational issue and if it works, who are you to judge my methods (Just saying). If you can kindly test on D8 and see if my fix works that would be great. Thanks!

Log in or register to post comments

Comment #11

xjm

she/her

English

CreditAttribution: xjm commented 14 September 2012 at 17:44

Issue tags:

+Needs issue summary update, +Needs screenshots, +Needs manual testing, +Needs backport to D7

Thanks for the patch @ishmael-sanchez!

@ishmael-sanchez is correct; we'll want to resolve the issue in D8 first and then backport it to D7. This is part of the process for fixing it in D7, so we can ensure that it doesn't break again once we fix it. It does not mean the patch does not get tested before going into D7--quite the contrary. Patches need to be tested even more thoroughly before they can go into D7 so that we can ensure we don't introduce regressions on production Drupal sites.

@Vc Developer, If you'd like to help test this issue in D7, you can create a D7 version of the patch yourself:

Make the same CSS changes from #1 in D7.
Create a patch with git.
Attach that D7 patc to this issue with a filename like 1699072-D7-do-not-test.patch. (The do-not-test part makes the testbot skip trying to apply the patch, since a D7 patch won't work on D8. See the fine print under the file upload.)

Tagging for manual testing, before-and-after screenshots, and an issue summary.

Log in or register to post comments

Comment #12

Shyamala CreditAttribution: Shyamala commented 24 September 2012 at 11:03

File	Size
after-multiple-user-tabs-bartik.png	44.12 KB
before-multiple-user-tabs-bartik.png	44.21 KB
after_multiple-user-tabs-bartik.png	44.12 KB
before_multiple-user-tabs-bartik.png	44.21 KB
after-multiple-user-tabs-seven.png	26.56 KB
before-multiple-user-tabs-seven.png	21.18 KB

Attached Screenshot of Seven & Bartik. The patch fixes disappearing tabs that can not be accessed in Seven. In Bartik the tabs are accessible before & after but the alignment needs to be fixed.

Log in or register to post comments

Comment #13

Shyamala CreditAttribution: Shyamala commented 24 September 2012 at 15:58

Title:

Not handling multiple tabs.....

» Multiple tabs on User Profile page on exceeds page width - doesn't wrap properly.

updating title

Log in or register to post comments

Comment #13.0

Shyamala CreditAttribution: Shyamala commented 24 September 2012 at 15:58

Issue summary:

View changes

update issue summary

Log in or register to post comments

Comment #14

YesCT CreditAttribution: YesCT commented 7 February 2013 at 06:21

Status:

Needs review

» Needs work

needs work based on #12

Log in or register to post comments

Comment #15

YesCT CreditAttribution: YesCT commented 7 February 2013 at 06:22

Status:	Needs work	» Needs review
Issue tags:	-Needs issue summary update, -Needs screenshots, -Needs manual testing, -Needs backport to D7

#1: tab-wrapping-1699072-1.patch queued for re-testing.

Log in or register to post comments

Comment #16

7 February 2013 at 06:24

Status:	Needs review	» Needs work
Issue tags:		+Needs issue summary update, +Needs screenshots, +Needs manual testing, +Needs backport to D7

The last submitted patch, tab-wrapping-1699072-1.patch, failed testing.

Log in or register to post comments

Comment #17

kevinsiji CreditAttribution: kevinsiji commented 15 March 2013 at 06:48

Issue tags:

-SprintWeekend2013

File	Size
seven-multiple-user-tabs-1699072-17.patch	360 bytes

after-multiple-user-tabs-seven.png	21.02 KB
before-multiple-user-tabs-seven.png	19.85 KB

It seems that wrapping issue has been fixed by now, and another issue has popped up.

The tabs of lower rows overlap with the upper rows and cannot distinguish them from each other. See the screenshot attached.

Before and after screenshots and patch attached.
Before:
Before

After:
After

Update: Screenshots embedded. Thanks @YesCT

Log in or register to post comments

Comment #18

9 March 2013 at 18:41

The last submitted patch, seven-multiple-user-tabs-1699072-17.patch, failed testing.

Log in or register to post comments

Comment #19

YesCT CreditAttribution: YesCT commented 10 March 2013 at 07:24

Issue tags:

+SprintWeekend2013

@kevinsiji thanks for those screenshots.

To ease reviewing and get reviews faster, embedding of the screenshots in the issue comment is recommended. (See contributor task http://drupal.org/node/1859584 for more info and tips). Also, dreditor makes it really easy to embed attachements.

Log in or register to post comments

Comment #20

kevinsiji CreditAttribution: kevinsiji commented 10 March 2013 at 16:18

Status:

Needs work

» Needs review

#17: seven-multiple-user-tabs-1699072-17.patch queued for re-testing.

Log in or register to post comments

Comment #21

Shyamala CreditAttribution: Shyamala commented 22 March 2013 at 04:01

Issue tags:

+SprintWeekend2013

Thanks kevinsiji for adding the screenshots. The tasks needs the below actions from you:

Post which browser the issue was tested on
Read through the instructions at http://drupal.org/node/1489010 for instructions on Manual testing. If there are any edits to the steps to reproduce, please add them as well
Remove the +Needs manual testing, Needs screenshot tags

Log in or register to post comments

Comment #21.0

Shyamala CreditAttribution: Shyamala commented 22 March 2013 at 04:01

Issue summary:

View changes

updating issue summary as per issue summary template

Log in or register to post comments

Comment #22

kevinsiji CreditAttribution: kevinsiji commented 18 April 2013 at 05:30

Issue tags:

-Needs screenshots, -Needs manual testing

The issue is tested on Chrome and Firefox in Ubuntu 12.10.

Log in or register to post comments

Comment #22.0

kevinsiji CreditAttribution: kevinsiji commented 18 April 2013 at 05:30

Issue summary:

View changes

added links for new contirbutors

Log in or register to post comments

Comment #23

YesCT CreditAttribution: YesCT commented 4 May 2013 at 23:50

Issue tags:

+Needs screenshots, +Needs manual testing

#17: seven-multiple-user-tabs-1699072-17.patch queued for re-testing.

Log in or register to post comments

Comment #24

star-szr

he/him

English

CreditAttribution: star-szr commented 24 May 2013 at 15:58

Issue tags:

-Needs screenshots, -Needs manual testing

Trying to fix tags.

Log in or register to post comments

Comment #25

BrockBoland CreditAttribution: BrockBoland commented 24 May 2013 at 19:26

Needs an issue summary: http://drupal.org/node/1427826

Log in or register to post comments

Comment #25.0

stevenovy CreditAttribution: stevenovy commented 22 September 2013 at 16:38

Issue summary:

View changes

Updated "Steps to reproduce".

Log in or register to post comments

Comment #26

stevenovy CreditAttribution: stevenovy commented 22 September 2013 at 18:31

Updated issue summary.

Log in or register to post comments

Comment #27

stevenovy CreditAttribution: stevenovy commented 22 September 2013 at 19:01

Title:	Multiple tabs on User Profile page on exceeds page width - doesn't wrap properly.	» Multiple rows of tabs not spacing properly in Seven theme
Assigned:	Unassigned	» stevenovy
Issue tags:	-Needs issue summary update, -SprintWeekend2013

File	Size
20130922_before.png	47.18 KB
20130922_after.png	48.2 KB

Tested patch in #17 in FF 23 and Chrome 29 on Mac OS X 10.8.5. Both worked fine. Embedding screenshots from FF with path /admin/appearance:
before:

after:

Will test it again with IE on a Windows machine.

Log in or register to post comments

Comment #27.0

stevenovy CreditAttribution: stevenovy commented 22 September 2013 at 19:01

Issue summary:

View changes

Updated issue summary.

Log in or register to post comments

Comment #28

heddn

English

Nicaragua

CreditAttribution: heddn commented 6 November 2013 at 18:28

Issue summary:	View changes
Issue tags:		+Needs reroll

Log in or register to post comments

Comment #29

idebr CreditAttribution: idebr commented 6 November 2013 at 21:07

File	Size
overlay-tabs.png	75.51 KB

Not sure about scoping of this issue, but tabs wrapping is still broken with overlay enabled (see screenshot in attachment)

Otherwise patch works as advertised :)

Log in or register to post comments

Comment #30

kevinsiji CreditAttribution: kevinsiji commented 15 November 2013 at 16:26

#overlay-tabs is positioned absolute in overlay-child.css of Overlay module. While growing vertically, absolute positioned #overlay-tabs cannot push the #overlay-content down, hence it cannot be wrapped properly in theme layer.

I feel, comment#29 should be filed against Overlay module.