More Seven Theme issues: #1986434: [meta] New visual style for Seven

This issue was merged with #2022695: Content header style update. Which includes moving the breadcrumbs outside of the content header.


Problem/Motivation

Tables have seen little issues, the only improvement we saw was to remove some of the distracting elements and improving the whitespace.

We developed Proposal: A Style Guide for Seven. This issue aims to introduce the proposed styling for tabs to core.

To quote the rationale provided:

  1. Left-align both primary and secondary tabs, including those attached to the overlay (see below). This is intended to resolve issue #763720: Visiblity of primary & secondary navigation by placing all tabs on the primary scan line (left margin).
  2. The negative space created where two rounded tabs are joined can be a visual distraction. The proposed style retains the border radius but removes the negative space by “extending” the tabs underneath one another.
  3. Secondary tabs are re-styled in an attempt to create a better connection between them and the primary tabs, while both linking them to and separating them from the content below.

The primary rationale for this redesign is the large disconnect we created by the right alignment of tabs, which were often missed. Similarly the secondary tabs because of their decreased visual prominence were missed even more. With a redesign of the overlay and modal, we can now accommodate a different style and placement of tabs.

Note: This issue doesn't cover tabs when overlay is open.
This will be addressed in #1953374: Implement Seven style guide for core overlay

Test pages

  • admin/structure/types/manage/article/display
  • admin/structure/views/view/files

Old summary - Redesign tabs for touch screens

Meta Issue:#1870944: [Meta] Mobile friendly admin pages

This is an issue that aims to implement part of the design from the Drupal mobile navigation prototype.

Tabs need to be bigger to be touch friendly. It's common for tabs on mobile interfaces to be fluid. Taking up as much space as is available.

Here are some examples of some basic styling I did in the prototype:

I actually had to add an additional class in that contains the number of tabs so we can set percentages correctly. That's how jQuery mobile does it and it seems to be the cleanest method.

Sub-issue of: #1137800: Increase minimum size of targets for touch screens

Files: 
CommentFileSizeAuthor
#294 drupal8.css-tabs.294.patch528 bytessun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,041 pass(es).
[ View ]
#290 Node edit - breadcrumb.png35.34 KByched
#280 new-tabs-1490402-280.patch41.38 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 63,404 pass(es).
[ View ]

Comments

If we use % for tab widths, how do we handle the issue when there's too many tabs for one line on a small screen?

Devel module likes to add a few tabs. Add in revisions, etc. and you can get 6 or 7 tabs on a node.

Issue tags:+Novice

Tagging novice, as I think anyone with decent CSS skills can participate in the discussion and code it up.

We need the ability to be able to tell how many tabs there are in the CSS. I think we need to add an "items-x" to item lists.

We can set media query rules like; anything more than "items-5" on widths less than 400px get split on to two lines.

Issue tags:-Novice+mobile-novice

Fixing proper mobile-novice tag

Status:Active» Postponed (maintainer needs more info)
Issue tags:-mobile-novice

The designs at http://nav3.d8mux.org/structure/menus/menu/ show a completely different styling for mobile vs. desktop. There's no direction on how to toggle that kind of switch in styling.

StatusFileSize
new288.56 KB

I think this is a lot more complicated than it seems at first glance, mostly for the reason of how do you handle showing five or six "tabs" on a phone?

Simply giving a percentage width will give you a problem akin to the menu bar on mobile.

So there are two reasonable options, imho:

1) Perhaps integrate the tabs with the slideout menu in development, or
2) Somehow switch the behavior of "tabs" on mobile to be grouped into a dropdown-style tab that reads "select a tab"

Image attached showing the content type config tabs as normal, 50% width, and drop down.

StatusFileSize
new12.69 KB
new24.48 KB
new19.09 KB
new22.78 KB

We went with @bohjan through the wider mobile navigation use cases but those choices it influences also the tabs.
We took a Admin / Structure / Content types / Article / Manage display menu item for the use case. Here are the mockups:

Case 1: What we have now

Case 2: Top navigation. Twitter Bootstrap-style navigation. Just 1st-level menu navigation is supported, the rest of the nav is provided by standard menu pages. Tabs turn into multilevel dropdowns for now.

Case 3: Left navigation. All menu levels are supported. Tabs turn into multilevel dropdowns for now.

Case 4: Left navigation with deep drill down. All menu levels are supported, tabs are also put on menu structure.

StatusFileSize
new37.75 KB

@kika, those look well though out. If I'm following, then any fix here will have to await a decision on which method to implement.

Assuming one of these will be use for this issue, not yet discussed is how to handle breadcrumbs as isrelatedly being discussed in #1058144 — particularly for things like the them admin pages.

Problem scenario:
problem scenario

I suggest considering the below attached, I think it's good for a few reasons:

1) A simple easier-to-understand flat menu structure that uses only two interaction metaphors: the dropdown for the main section and flat lists for tabs and breadcrumbs. It might be a couple more clicks, but better to click through a second mobile page than fiddle with dropdowns.

2) Adds a "header" breadcrumb to the title so you can remember where you came from and navigate back up stream — thus recreating functionality traditionally done by the breadcrumb/tab combination.

3) Accommodates breadcrumbs, something not addressed in the other mockups.

Solution

@kika and @bohjan - I really like the idea of turning the top level tabs into select elements as seen here:

One other question: How should we handle local tasks(View, Edit, Delete, etc)? Perhaps use a select box above the Page Title for the main tabs(Manage Fields, Manage Display,etc) and add a select box below the Page Title for local tasks?

I like how it keeps everything in context and visible in Case#2. If we went with Case#3, I think the page specific context would be lost. I think page specific tabs(like the main tabs) should be visible, but the site navigation(links that are the same across the site) could be hidden behind a menu button/s of some sort.

I like the Case #2 and #3 left sliding menus, but I have concerns that showing multiple nested levels of menu items would get to long. I think an ipod like drilldown sliding navigation would help with the menus getting to deep and long. See http://www.aids.gov as an example. You will have to resize your browser to see the Menu button where you can drill down the menu items.

Status:Postponed (maintainer needs more info)» Active

@gmclelland From a technical design, I like the idea of dropdowns for navigation, but I dislike them for UX.

Imho, dropdowns and tabs are very different. With a dropdown the options are effectively hidden from the user, very much unlike tabs.

Granted, when we discuss mobile, we're talking about a squished down and compromised small-screen platform.

Still, #2 I think is the best of the bunch, even though I like my adaptation more.

@KenHawkins - your proposed approach makes sense, will think about it some more. Breacrumbs and / or backbutton is a valid point but I don't not see it as a critical navigation element.

I wonder if we can draw inspiration from other places, it seems the select list is most viable. But it feels a bit techy as solution.

Category:task» bug

Reclassifying

Just a note that three horizontal lines is becoming the standard for buttons that open up menus on mobile devices.

If there are just two or three tabs, personally I'd rather have them visible on screen, rather than having them buried in a select menu, even if it means I need to scroll more. I can understand how that could get unwieldy with six or seven tabs. Is there any data on what are the average number of tabs in use on most pages? And what the high-end of number of tabs is likely to be?

StatusFileSize
new1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 47,566 pass(es).
[ View ]

I made a patch! Try out a demo on your phone here: tabs.d8mux.org

Status:Active» Needs review

This is awesome. I love it.

StatusFileSize
new2.04 KB
FAILED: [[SimpleTest]]: [MySQL] 47,765 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Based on some quick fire feedback, our expectation that the horizontal tabs being undiscoverable were confirmed.

Luckily this is the same problem that early iPhone apps ran into with this pattern, we can draw inspiration from them and I am confident we can make this interface element intuitive out of the box.

Some techniques early iOS apps used included:

  • A gradient fade into the background colour.
  • An arrow pointing to the right
  • An onload animation that slides the tab in from the left, which is then disabled after the users use the UI element.

This patch only implements the gradient, I'd rather solve it with less.

You can once again see this UI on your phone at http://tabs.d8mux.org

StatusFileSize
new2.06 KB
PASSED: [[SimpleTest]]: [MySQL] 48,580 pass(es).
[ View ]

Oops sorry, last patch sucked. This one rocks!

Status:Needs work» Needs review

#20: drupal-mobile_tabs-1490402-20.patch queued for re-testing.

#17 is very cool. would love if we could have a simple solution like this

Issue tags:-#d8mux+d8mux

Issue tags aren't Twitter hashtags. :-)

Issue tags:+d8mux-admin

+ tag d8mux-admin

+ tag d8mux-admin

Status:Needs review» Needs work
StatusFileSize
new94.42 KB

Hi.

Tested the #20 patch and it looks great in Chrome, Safari and my iPhone.

But in Firefox and Opera appears a rare gradient box as shown in the attached image.

Status:Needs work» Needs review

Needs testing on mobile devices. Not sure if FF desktop is relevant, so I'd like to keep this for review. @Lewis Nyman this is awesome!

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new30.85 KB
new30.34 KB
new30.46 KB

Tested #20 patch. Now the scroll-bar helps to navigate all the links in menu.
Tested in Android 4.0 & IOS 6

StatusFileSize
new2.13 KB
PASSED: [[SimpleTest]]: [MySQL] 50,734 pass(es).
[ View ]

I've rerolled the patch against head and I've also made the CSS touch screen specific it's a pattern that doesn't really make sense on a mouse and keyboard so let's avoid desktop devices altogether.

The styling itself has not changed — I've only prefixed the selectors with '.touch' — so the previous tests should stand, if people are happy then let's mark it back to RTBC.

Status:Reviewed & tested by the community» Needs review

Tested #32 path.
This patch also works well like #31

Status:Needs review» Needs work

The last submitted patch, drupal-mobile_tabs-1490402-32.patch, failed testing.

Status:Needs work» Reviewed & tested by the community

looks good to me too :)

StatusFileSize
new49.35 KB

Odd behavior on Chrome 25 for Linux from http://tabs.d8mux.org/. Is this expected?

Screenshot on Chrome 25 Linux Desktop

Status:Reviewed & tested by the community» Needs work

That looks like a needs work. :)

Status:Needs work» Reviewed & tested by the community

The demo is not up to date with the latest patch.

Also, test mobile patches in an emulator at least, that won't work otherwise.

(edit) All the styles have been scoped with .touch in the latest patch so modernizr needs to detect that the browser is touch enabled to show the proper styling. Scrolling renders much differently on real mobile devices and desktop: hence the emulator.

Priority:Normal» Major
Status:Reviewed & tested by the community» Fixed

This looks good to me. Committed to 8.x.

It would be nice if we could have a more generic solution but until then, this actually fixed a pretty major bug in my opinion.

I don't disagree with committing this patch, since obviously not being able to click tabs at all is a pretty big problem.

Just so my feedback is logged somewhere, though:

a) that gradient looks awfully cheesy to me, sorry.
b) nowhere else in any website/IOS/Android App I've seen does gradient mean "something you can swipe."
c) the behaviour, therefore, is not very discoverable. :( Would've much preferred to see an ">" arrow or something like that, though not sure how to reconcile that in such a small space.
d) this fixes it for touch devices, but not for narrow devices. Collapsing admin/structure/fields still looks like total ass in Chrome.

So I'd love to see another revision on this, personally. I'm fine being out-voted on it though.

nod_ also pointed me at #1880488: Make menu collapsible on small screen resolutions which is worth cross-linking here, since it would solve it for both narrow screens + touch devices.

Status:Fixed» Needs work
StatusFileSize
new93.52 KB
new374 bytes
new2.89 KB

Rerolled to deal with a horizontal scrollbar on the page. This occurs because Seven declares summary elements to be width 100%. But system.theme.css gives summary elements left/right padding. So they bust out of their containers. The summary element is already a display block, so it doesn't need its width set to 100%. I verified that removing the width doesn't break the ellipsing.

Screenshot of the Seven admin theme at a narrow width with a horizontal scrollbar present even though it should not be present.

Upon further thought, this is not the issue that Dave Reid raises in #37. For that issue, we probably need to futz with the background-origin value to the gradient image to get it off the scrollbar. Just a guess, I haven't looked into it yet because I don't have Chrome 25 running on Linux at the moment. I need to set that up.

[edit]

This tweak has been extracted and moved to #1896894-1: Width declaration on summary elements in the Seven theme trigger a horizontal scrollbar at narrow widths

Priority:Major» Normal

Status:Needs work» Fixed

Righteous. Ugh. Maybe we can put the theme's name in the issue or set the Component correctly so searching for dupes actually yields results? Sorry, a bit frustrated I spent an hour tracking this down when it was already being adressed. Glad it's solved though. I closed my issue (#1896894) as a dupe.

StatusFileSize
new37.13 KB
new45.11 KB
new48.77 KB

Why can't we use the same pattern from Bartik's main menu?

bartik-wide.pngbartik-narow.pngbartik-compact.png

StatusFileSize
new78.82 KB
new41.43 KB
new24.43 KB
new177.22 KB
new180.53 KB
new100.9 KB
new64.52 KB

@webchick

Thanks fo the feedback, I was waiting for someone to question this method, there are definitely other options available.

a) We can tweak the gradient, making it softer or more subtle.
b) This patterns has actually become surprisingly common, strangely in some situations, with no affordances all at! I've attached a few examples below.
c) If the pattern is not discoverable at all. I think there is a lot we can do to make it more discoverable, some very early iOS apps did some crazy stuff to introduce this pattern to users.
d) This is true, if we consider narrow and non-touch devices to be a use case — I think we should — then we should decide if we want to implement a solution that fixes them both in one go or a separate solution taking advantage of the different conventions of touch and non touch devices.

To answer general thoughts on why even implement a different solution to Bartik:

Vertically stacking the tabs would be a huge PITA when browsing across multiple screens, you wouldn't even be able to see any of the content on most pages without scrolling and has been considered for many years to be a poor mobile web UX. (See: http://www.w3.org/TR/mobile-bp/#cm http://alistapart.com/article/organizing-mobile) It would be shame, given how much work we spent on finding a good solution for the toolbar, if we abused the vertical space we saved.

The active tab is a very useful indicator of where you are in Drupal's IA, a solution that hides all of the navigation removes this advantage and could hurt UX. I was reluctant to do this because Drupal's IA can be confusing at the best of times.

Bartik's tabbed navigation is its primary navigation, the tabs in the admin section are secondary navigation. At the time the design of the toolbar was still up in the air and ideally we wouldn't want the two patterns to conflict with each other. This is still a consideration now that the new toolbar design is in core.

Status:Fixed» Needs work

Lets keep this open, I agree its a solution - but probably not the best solution. I would really be interested to usability test this once, I am not sure if all the other examples are necessarily well researched decisions or just "the only way we can do it atm, solutions" :)

It sounds like we are unsure of the best approach here. IMO, we should fix the critical bug (can't get to some tabs using a Mobile device) using the patch we have. Folks with better ideas can advocate for them in follow-ups.

We already did, don't worry. :) This is just a normal "needs work" to see if we can come up with something better.

StatusFileSize
new18.06 KB

Just a thought: gradient makes more sense, if we don't keep our tab style? and try something like below screenshot:

tab-design.png

Or

Only local images are allowed.

Bojhan brought my attention to this issue from my work on the Seven style guide. I want to try something other than the solution arrived at above. Here’s a demo (unfortunately only tested in Chrome, so YMMV). I’d love to get feedback:

http://drupalcode.org/sandbox/ry5n/1932040.git/blob_plain/refs/heads/res...

+1 for 52. That's beautiful.

#52 looks great

I agree that the current solution is not ideal. On the other hand the big benefit is that it doesn't require any JS.

Performance is a feature and we're starting to throw that out the window. That'll be potentially on every admin pages it will have a lot of impact. It's just a heads up that it'll be an important part of the review.

For the record, I do like the prototype :)

StatusFileSize
new3.25 KB

...and here's a screenshot for the record:

tabs for touch prototype from #52

The design looks solid. It would be nice to see some more finesse to the media queries, having just one breakpoint feels clumsy, we're hiding more than we need at wider viewports just because we have to do something. We definitely don't need to unfloat secondary tabs until much narrower and we don't need to hide every primary tab at 800px to get them to fit into that width. One of the benefits of the current solution is it only hides what it needs to and nothing more. I think we could achieve this with this solution without a huge amount of Javascript.

Good work!

@LewisNyman I totally agree that this could be way better in dealing with what can fit and what can’t. Media queries aren’t really suited to this kind of thing at all – what we really need is a test to see if all the content can fit or not, etc. But that’s content- and container-dependent. I’ll try to figure out a simple way to improve this with as little JS as possible.

Status:Needs work» Needs review
StatusFileSize
new53.63 KB

So… it took some time, but I have something solid now along the lines of #57 (demo below). I’d love to get a code review (RE: _nod’s concern in #55) but since this is sandbox code and not a core patch, I’m not sure the best way to share the code for review. I could post links to the sandbox git tree… advice?

The demo:
http://drupalcode.org/sandbox/ry5n/1932040.git/blob_plain/7f259b9e419b2d...

Screenshot:
Auto-responsive tabs

This direction feels fine, to me and much better than our current solution.

@Bojhan Cool. This was what was holding up the Overlay for patch, IIRC.

Oh my this looks and behaves very well! Who can help make the patch?

StatusFileSize
new44.15 KB

@ry5n: There's a small bug in the second item - there's a state in between the full horizontal display and the vertical display where it stacks the tabs onto multiple rows:

This was on the latest Safari on OSX 10.7.

Status:Needs review» Needs work

Needs work because needs patch. Thanks for testing DamienMcKenna.

@DamienMcKenna Thanks. The JS that calculates the 'breakpoint' probably just needs a bit of a manual buffer, say 5px. I’ll post an update. I will need help converting this to an actual patch, esp. PHP and adapting the JS.

StatusFileSize
new420.73 KB

I have noticed a problem in Firefox v.21 y Opera v.12.15 (yes I know nobody cares about Opera :P) with Mac OSx. It seems tabs appears collapsed in full screen view as you can see in the screenshot. Maybe it's due some javascript errors and warnings.

I’ve added a viewport meta tag and fixed some JS bugs in my implementation. Demo of the new commit is at http://bit.ly/YLcw6U

There’s also a discussion over at #1880488: Make menu collapsible on small screen resolutions starting at #63: weighing this implementation among two others for general use in core.

Wow, that demo is impressive!!! Awesome stuff.

Ok, now the problem in Firefox reported in #68 is solved. There are still some errors but is working really well. Great work!!

@rteijeiro, @tstoeckler Thanks. Any chance you could post the remaining errors? I’ll take another look.

Also, since you’re interested, this other issue is about whether we should choose a common responsive nav implementation for core (Bartik/Seven): https://drupal.org/node/1880488, with this one as a candidate. Would be good to get more input.

Sorry, here you are the error logs in Firefox 21 with Mac OS:

EDIT: put the log in a txt file in comment below.

@rteijeiro: Hey Ruben! Can you please move the lengthy comment to a .txt file, upload the file and delete the comment. Thanx in advance.

StatusFileSize
new14.05 KB

Sure. Sorry for the mess... :)

No problem mate ;)

Thanx for keeping the issues clean. Long pages take time to load - especially in issues with lots of comments.

tkoleary digitally high-fives ry5n.

Thanks everyone for the positive feedback. I’m not able to take this any further right now: I need to take a break from core for health reasons. This needs a bit of polish and integration with Drupal’s JS architecture. If anyone wants to pick this up and run with it, please do.

StatusFileSize
new6.58 KB
PASSED: [[SimpleTest]]: [MySQL] 57,974 pass(es).
[ View ]

Here's the beginning, the no JS tabs.

Next step, write the Javascript to detect when there's enough room to display the menu horizontally.

StatusFileSize
new12.19 KB
new18.77 KB
PASSED: [[SimpleTest]]: [MySQL] 57,735 pass(es).
[ View ]

Today I got the JS to apply the horizontal class. I don't really understand all of it but we'll need nod_ to review the architecture later.

Next step: Add the "desktop" CSS

Thanks Lewis for picking this up. I wish I’d written more code comments. Even though I’m supposed to be on hiatus I’m happy to answer specific questions about this code. (BTW the dom-utils in there can probably be scrapped and replaced with simpler Drupal-native methods.)

Status:Needs work» Needs review
StatusFileSize
new23.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,117 pass(es).
[ View ]
new9.97 KB

No problem, I'll ping you on email if we need your input so you don't have to keep checking back.

This patch is getting closer now. I've implemented the styling for both mobile and horizontal states for secondary and mobile, along with the collapsible functionality. I made a bit of a hash of the CSS, could do with a few more eyes.

Had a quick look at it, perf is pretty good. Takes about 20/30 ms to init on desktop. And it seems like the code can be simplified.

Nice work :)

Title:Redesign tabs for touch screensRedesign tabs
Category:bug» task
Issue tags:+styleguide

With the advent of the style guide, the scope of this issue has been expanded. It makes no sense to split the redesign for desktop and mobile into two issues.

Issue summary:View changes

Added link to meta issue: #1870944

Issue summary:View changes

Updated issue summary to match the new title

StatusFileSize
new23.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,603 pass(es).
[ View ]
new2.46 KB

We needed a reroll in light of #1363112: Simplify names of "element-x" helper classes.

I also noticed the the hover style wasn't applying correctly.

Can this get a screeny? Last time I tried this it was quite buggy.

DO LIKE.

Great job on this :)

Status:Needs review» Needs work

I love this design :-)
The secondary tabs need work to match Seventy-Eight, their underline as shown in the screenshots, needs to be 2px, and it should not extend out to the anchor's padding. Then when they become stacked it looks like you missed that display, you would need to add more items to see it (too tired for detail or patch).

Another point, in Seventy-Eight, the right pointing arrow is an icon font, which I understand we might be able to use at some point, but this hasn't been settled. Here we are using a character entity, which is a great option, it's just unfortunate that it doesn't look as good as the icon font.

Status:Needs work» Needs review
StatusFileSize
new23.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,730 pass(es).
[ View ]

Here's a re-roll in light of #2015789: Remove language_css_alter() (RTL stylesheets) in favor of HTML 'dir' attribute

All of echoz's points in #89 are valid.

It looks like this needs some design for the non-overlay styling. Ideally its part of the content header, not separate.

@Bojhan I've never seen a design that shows the tabs inside of the content header? See one of our sandbox examples

On Bojhan's request I tried moving that tabs into the content header, so they matched the same placement as the overlay design.

Screen Shot 2013-07-02 at 22.31.53.png

The contrast could be better but we can't really increase the brightness because of the hover effect. The bottom border line is a bit jarring.

Screen Shot 2013-07-02 at 22.32.08.png

Restricting the line to the width of the tabs seems a bit weird also.

Screen Shot 2013-07-02 at 22.36.45.png

Removing the line isn't an option.

Screen Shot 2013-07-02 at 22.40.05.png

There's no way this design meets WCAG 2.0

Assigned:Unassigned» Bojhan

Let me pick this up. It's likely we need a different color scheme for non-overlay styling, after all the background color is quite different so we need it to match differently.

Assigned:Bojhan» Unassigned
StatusFileSize
new56.1 KB
PASSED: [[SimpleTest]]: [MySQL] 56,749 pass(es).
[ View ]

I've tweaked the colours a little bit on the non-overlay tabs. And I've managed to get the bottom border line to stretch across the whole of the screen.

Bojhan feel free to chime in with new colour changes.

@LewisNyman

This looks awesome. I'm going through it now and testing edge cases. Views is one. Secondary tabs are all messed up. IMHO we should remove views' anti-pattern here and just use the base style.

StatusFileSize
new24.9 KB

@LewisNyman

Heres what views looks like:
image

StatusFileSize
new41.55 KB

And here's another. Assuming I'm seeing > because the iconfont is not installed? Perhaps the character should be → ?

image

StatusFileSize
new24.18 KB

And this:
image
Secondary tabs are larger than primary.

Thanks Kevin.

Actually the '>' is just a placeholder until we decide on an icon that is GPL friendly.

I think dragging Views UI into this issue is going to overload it. I'd love to tackle the consistency of Views UI as a separate initiative after this one. For now we should make sure View UI looks the same.

I think dragging Views UI into this issue is going to overload it.

As long as it doesn't cause a visual regression, like #100 does.

StatusFileSize
new5.3 KB
new62.15 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I fixed the views issues raised in #100. I also rolled back the .active to .is-active class change because it's way out of scope. It's covered in #2031641: Change active class in l() function to is-active

I guess this needs a re-roll or correction. With fresh install, after applying this patch i get a white screen with this php Fatal error: Cannot redeclare seven_menu_local_tasks() (previously declared in PATH_TO_seven.theme:47).

Same here, tried to work on it today - but cant get it to apply at all.

Status:Needs work» Needs review
StatusFileSize
new59.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-112.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled. There were a few commits that knocked this out.

Status:Needs work» Needs review

I think this is still failing, it doesnt apply at simplytest.me.

StatusFileSize
new59.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,487 pass(es).
[ View ]

Re-roll

Im throwing my own stuff here as well. Many of the ones above look really good already.

@LewisNyman

Bog noted in #102 is still present in the latest patch.

Also the tabs still break at less than 320px.

I don't think we've considered supporting devices below 320px, are there popular devices below that width? We should document the device ranges/features we support

One thing to consider. If you're supporting devices with 320px, there is a use case to consider making sure the interface doesn't totally break below 320px. Websites are often used not just within a browser, but also within apps that can add extra chrome on the side, reducing the actual viewport below 320px.

That said, I saw a recent stat that nearly 97% of mobile usage is on devices with at least 1.5x resolution. (In theory that could vary by geography.) If that's the case, the likelihood of actually seeing viewports of even 320px is pretty low.

Hmm. Just checked the stats for the main site I manage. 7000 of the 17000 mobile visits were at 320 resolution. I see about 70 visits below 320. But that's screen resolution not viewport.

Hard to say how many people would administer a site within an app that brings chrome below 320. My stats show 25% of iPhone site visits are within apps, rather than just in Safari. Is it possible that none of those would be for administering a site? Maybe.

Anyways, short version is that there is a possible use case for people at 320 resolution to have a viewport below 320 where the tabs might breaks.

Ok, it's worth giving it a go. We should to draw the line somewhere though, we can't make 100px widths look good.

StatusFileSize
new45.4 KB

I'm not looking forward to administering views from my cheap rip-off iWatch :)
That is, below 320 we'll have much bigger issues than these tabs breaking.

What I miss here is *how* broken the tabs are below 320. Unusable? I see the ">" mis-aligning to the bottom but that seems to be the case at any width where that display is used (latest Firefox).

StatusFileSize
new59.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,347 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new2.01 KB

I've fixed the Firefox arrow bug and I've removed the arrow and collapsible functionality on very small screen sizes, that provides a bit more space for longer menu titles.

Status:Needs work» Needs review
StatusFileSize
new1.41 MB

When overlay is turned *off*, the tabs are first rendered in "smartphone" mode and than they get placed correctly. It's very very very anoying to see this happen and feels like a serious regression.

I don't minder thinking mobile first when it comes to design but it shouldn't mess with the desktop experience like this.

Example: go to admin/people/permissions. Or look at the attached screenshot.

Hm, was this regression only introduced in the last patch? I definitely don't want to negatively effect desktop users to cater to a very small usecase.

No it's happening since the initial patch that has the js included. Always been that way (as far as I can remember), just told this now because I thought it would be fixed by now.

Status:Needs review» Needs work

Assigned:Unassigned» Bojhan

I will work on the visual design issue for now, tabs are not rounded - which is a problem.

There is a small overlay test bug, btw.

StatusFileSize
new42.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new42.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch overlay-new-tabs-1490402.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new101.67 KB
new110.13 KB
new110.13 KB

Reviewing this, in general I think we should bring back the rounded corners this was part of the original design work and removing it to me feels like removing a fundamental part of the design. I uploaded two patches, since I couldn't figure out how to get the border bottom to exist consistently.

# Overlay design
Screen Shot 2013-08-24 at 8.21.36 PM.png

The border is still somewhat broken, ideally it should run to the far right of the overlay - not stop after the last tab.

# No overlay design
Screen Shot 2013-08-24 at 8.24.52 PM.png

The only thing I am unsure about is the active tab, in the ideal situation the background of the tabs, should run in the background of the active (as designed in the original design - https://groups.drupal.org/node/283223#Overlay ) but I see as that being hard to do in CSS.

Also removed a bunch of spacing in both, no idea why that was there.

Assigned:Bojhan» Unassigned
Status:Needs work» Needs review

This is ready for review and further work, I'd be nice to get this in an acceptable state before Prague.

Assigned:Unassigned» mdrummond

I'll try to run a reroll on this.

Status:Needs work» Needs review
StatusFileSize
new42.03 KB
FAILED: [[SimpleTest]]: [MySQL] 59,003 pass(es), 1 fail(s), and 47 exception(s).
[ View ]

Rerolled this. Should work now. (There were two patches on #131 but they both look identical.)

Issue tags:+styleguide-add-icons
StatusFileSize
new30.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,144 pass(es).
[ View ]

The patches between #123 and #131 have about 20kb size difference. We're missing some JS files.

I've removed all code related to #2022695: Content header style update and #1953374: Implement Seven style guide for core overlay in order to simplify this patch. Let's crack the basic non-overlay implementation before we move on. We need to implement the icons per #2032773: Use Libricons (icon font) in Seven, consider using it more broadly in core

Status:Needs review» Needs work
StatusFileSize
new73.2 KB
new60.76 KB

Patch applies well and it looks pretty good.

I have noticed that tabs change form vertical position to "is-horizontal" position when loading. It seems a bit weird because the change is too slow and easy to notice.

Also tabs are not working well with Overlay.

Attached screenshots.

tabs.pngtabs-overlay.png

Assigned:mdrummond» Unassigned
Status:Needs work» Needs review
StatusFileSize
new31.82 KB
PASSED: [[SimpleTest]]: [MySQL] 59,234 pass(es).
[ View ]

Ok, I've tried to deal with the FOUC a bit better and added the new icon. Let's worry about the overlay after we get the standard view down.

StatusFileSize
new13.18 KB
new11.66 KB

Nice work Lewis! It's great to see icons (and svg too!) for the chevrons. I noticed the png version did not come out right though.

I mainly wanted to note that my feedback from #89 about the secondary tabs still stands, and it will be easily missed unless more of them are enabled (manage display) until they stack vertically in a narrow viewport. They lack styling specific to seventy-eight, no backgound, left borders, and active state.

Secondary tabs stacked

secondary-tabs-stacked.png

Seventy-Eight

seventy-eight-sec-tabs-stacked.png

To test stacked secondary tabs, view Manage Display and enable a few more under Custom Display Settings until they stack on a narrow screen.

@echoz Could you hit me on IRC, I dont fully understand your concerns.

@Bojhan, sorry I missed you on irc. This is just a heads up that the secondary tabs when *stacked* have not been styled, which is understandable because we don't normally see them stacked, but this display was accounted for in the prototype. To test stacked secondary tabs, view Manage Display and enable a few more under Custom Display Settings (RSS, etc) until they stack on a narrow screen. I show this in #141 screenshots.

Lewis acknowledged this in #90 so it seems he gets it, like I said, just a heads up to keep that on the table in such a long issue.

Assigned:Unassigned» LewisNyman
Status:Needs review» Needs work

@echoz Nice catch, forgot about that, patch incoming.

Assigned:LewisNyman» Unassigned
Status:Needs work» Needs review
StatusFileSize
new32.11 KB
PASSED: [[SimpleTest]]: [MySQL] 58,842 pass(es).
[ View ]
new2.53 KB

Sorted out the secondary tabs and a few other things that came up.

StatusFileSize
new30.99 KB
PASSED: [[SimpleTest]]: [MySQL] 58,526 pass(es).
[ View ]
new17.46 KB
FAILED: [[SimpleTest]]: [MySQL] 58,648 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Now #2022695: Content header style update is RTBC, and the two issues need to be committed together, you now need to apply the two patches together to test this issue effectively. This is the best way I can think of handling this right now, any suggestions are welcome.

I'll add the latest content header patch here, with the new tabs patch that relies on it.

It still looks like its bugged. The bottom border should stretch the whole page.

Status:Needs review» Needs work

This needs work, the border bottom is still wrong

StatusFileSize
new30.3 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-153.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new17.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,639 pass(es).
[ View ]
new2.53 KB

Ok, I've made some spacing changes and others discussed wit Bojhan and Roy. I've re-attached the content header patch for ease. Please try applying them in the order attached.

I think the patch will fail because it relies the content header patch. I will not apply just against HEAD. This still needs review

We should remove the 1px, 1px border width. The rest looks great now! :)

Before

Screen Shot 2013-09-27 at 1.17.23 PM.png

After

Screen Shot 2013-09-27 at 1.16.06 PM.png

Assigned:Unassigned» mcjim

Assigned:mcjim» Unassigned
StatusFileSize
new11.57 KB
new333 bytes
new30.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-159.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Border-width changed as per #157

Haven't set to Needs Review as it depends on the content header patch, but it needs review.

Horizontal secondary tabs with border changed to 0 0 2px

Ah sorry, depends code-wise on the other one.

Status:Needs work» Needs review
StatusFileSize
new48.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,229 pass(es).
[ View ]

Here's a combined patch of both applied together. Just so we can make sure we are passing tests.

The user experience with latest patch is still not good...
First of all there is a strange padding/margin under the primary tabs.

I still see the switch between mobile and desktop mode resulting in incredible annoying flickering. I don't get why others don't mention this...

So it goes like this:
- no tabs
- mobile styled tabs
- desktop styled tabs

o_O :( :( :(

Yhea, I think the margin/padding in the latest patch is bugged - we fixed that before.

The annoying switching is indeed still there, anyone ideas how to resolve that?

StatusFileSize
new47.3 KB
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]
new30.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-166.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.12 KB

I've tried reducing the flash even more, by setting the tabs to display horizontally by default. This means that the solution is no longer mobile first, with no javascript, on narrow screens, the tabs will look broken but still usable.

Yhea, I think the margin/padding in the latest patch is bugged - we fixed that before.

I'm not sure what this means, can someone take a screenshot?

That'll need some JS review, not sure what $.fn.dataOptions comes from or what it's supposed to be doing as well as some other code in there.

The opening and collapsing isn't fluid. One could say it looks bad.

Status:Needs review» Needs work
StatusFileSize
new109.08 KB

I was hoping to do in-depth manual testing, but the very first thing I tried was completely broken :(
Screen Shot 2013-10-01 at 17.17.15.png

Status:Needs work» Needs review

This patch does not cover the overlay :) We're going to cover that in #1953374: Implement Seven style guide for core overlay unless the overlay gets removed. That's mentioned somewhere in this issue. We probably need an issue summary update.

I found some minor problems, which I think are still important enough to require being fixed.

Screen Shot 2013-10-01 at 17.50.55.pngScreen Shot 2013-10-01 at 17.50.58.pngScreen Shot 2013-10-01 at 17.52.27.png

Also note that when moving from the first to the second screenshot, the buttons effectively become 1px taller! I doubt this is intentional?

Issue summary:View changes

A few tweaks

#169: @LewisNyman:

This patch does not cover the overlay :) We're going to cover that in #1953374: Implement Seven style guide for core overlay unless the overlay gets removed. That's mentioned somewhere in this issue. We probably need an issue summary update.

Issue summary updated ;)

.tabs__tab.active {margin: 0 -4px; } would solve the horizontal problem with rounded corners.
For the vertical thing, just need to have either a media query or maybe better to have JS add a new class to remove rounded corners there.

Status:Needs work» Needs review
Issue tags:-Usability, -mobile, -needs accessibility review, -styleguide, -d8mux-admin, -styleguide-navigation, -styleguide-add-icons
StatusFileSize
new47.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,808 pass(es).
[ View ]
new30.2 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-174.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.46 KB

Ok, I've fixed those small visual bugs with the rounded corners. I've also disabled the animation for opening/collapsing on mobile. It's not essential and seems to cause performance issues.

I can confirm that #174 solves all problems I pointed out in #170, with the exception of the second screenshot. But maybe that is intentional? It's definitely the smallest of the problems I found.

StatusFileSize
new47.27 KB
PASSED: [[SimpleTest]]: [MySQL] 58,738 pass(es).
[ View ]
new30.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-177.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ack no, sorry I missed that on my screen.

Status:Needs review» Reviewed & tested by the community

Patch in 177 looks good to me. Not sure why the upper one failed when the combined one passed? Ignoring that I think the combined patch is RTBC.

  1. +++ b/core/misc/jquery.collapse.js
    @@ -0,0 +1,98 @@
    +;( function( $, window, document, undefined ) {

    This looks wrong.

  2. +++ b/core/misc/jquery.collapse.js
    @@ -0,0 +1,98 @@
    +    this.options = $.extend( {}, defaults, options );

    Almost everywhere in core, and in our coding standards AFAIK, we don't have spaces after parens.

  3. +++ b/core/misc/jquery.collapse.js
    @@ -0,0 +1,98 @@
    +    if (opts.target) {
    +      if ( opts.target.jquery ) {

    Same here, and throughout. Let's remove those please

  4. +++ b/core/misc/jquery.collapse.js
    @@ -0,0 +1,98 @@
    +      } else {
    ...
    +    } else {

    Two lines.

  5. +++ b/core/misc/jquery.dom-utils.js
    @@ -0,0 +1,59 @@
    +;( function( $, w, undefined ) {

    Oh, I think I get it now. Is it to help when aggregating possibly malformed files?

  6. +++ b/core/modules/shortcut/css/shortcut.icons.css
    @@ -25,42 +25,29 @@
    -  background-position: -12px -12px; /* LTR */
    +  background-position: -60px top; /* LTR */
    ...
    [dir="rtl"] .remove-shortcut a:focus .icon,
    [dir="rtl"] .remove-shortcut a:hover .icon {

    Guessing you should update the RTL

  7. +++ b/core/modules/system/css/system.module.css
    @@ -372,3 +372,60 @@ tr .ajax-progress-throbber .throbber {
    +/*

    /**

  8. +++ b/core/modules/system/css/system.module.css
    @@ -372,3 +372,60 @@ tr .ajax-progress-throbber .throbber {
    +  -moz-appearance:    none;
    +  appearance:         none;

    Please remove the extraneous spaces

  9. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -457,20 +457,30 @@ td.group-title {
    -ul#views-display-menu-tabs {
    +.views-displays .tabs.secondary {

    Can someone post screenshots of these Views UI changes?

  10. +++ b/core/themes/seven/js/nav-tabs.js
    @@ -0,0 +1,20 @@
    + * Initialise the tabs JS.

    Initialize :)

  11. +++ b/core/themes/seven/seven.theme
    @@ -34,12 +34,81 @@ function seven_library_info() {
    +    $variables['primary']['#prefix'] = '<h2 class="visually-hidden">' . t('Primary tabs') . '</h2>';
    +    $variables['primary']['#prefix'] .= '<nav role="navigation" class="is-horizontal" data-drupal-nav-tabs="collapsible: true;">';
    +    $variables['primary']['#prefix'] .= '<button class="reset-appearance tabs__tab tabs__trigger js-nav-tabs__trigger">&bull;&bull;&bull;</button>';
    +    $variables['primary']['#prefix'] .= '<ul class="tabs primary js-nav-tabs__target clearfix">';
    +    $variables['primary']['#suffix'] = '</ul>';
    +    $variables['primary']['#suffix'] .= '</nav>';

    Do we ever abuse prefix/suffix like this eleswhere? This looks pretty nutty.

  12. +++ b/core/themes/seven/seven.theme
    @@ -34,12 +34,81 @@ function seven_library_info() {
    + **/
    +++ b/core/themes/seven/style.css
    @@ -197,9 +197,21 @@ pre {
    + **/

    only */

  13. +++ b/core/themes/seven/style.css
    @@ -255,151 +267,247 @@ pre {
    +/* TODO: Make this number less magic */

    This and all other todos should be @todo, not @TODO:, and should have issues opened for them

Status:Reviewed & tested by the community» Needs work

Also, this is still tagged "needs accessibility review", if that's done the tag can be removed, but this isn't RTBC if not.

I asked the a11y team to chime in

Wow, I had no idea we were adding any JS at all, let alone so much custom JS (I haven't done a code review yet). At least, AFAICT it is all custom JS. If so, it needs docs. Right now there's zero docs in all that JS, and as tim.plunkett says, it does not at all conform to Drupal's JS coding standards.

Thanks for the reviews guys, almost there. I'm looking forward to a JS code review. I'm pretty sure Ry5n wrote all the core/misc libraries to be independent plugins from Drupal. I actually don't know if all of them are written by him or not. I'd like to host them on Github and include them as a third party lib if possible. It doesn't mean it can't conform to Drupal's coding standards though.

StatusFileSize
new49.92 KB
FAILED: [[SimpleTest]]: [MySQL] 59,850 pass(es), 0 fail(s), and 74 exception(s).
[ View ]
new32.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-184.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.97 KB

I've only addressed the RTL styling in this patch. I noticed an RTL bug in the content header patch while doing this. Switching development back over to #2022695: Content header style update until it is back to RTBC/postponed.

StatusFileSize
new16.37 KB

I added the patch in #184 to STM and it didn't display right. I just did this here:
http://s47972b1cb4c951f.s2.simplytest.me/node/1#overlay=node/1/edit&over...

Messed up edit tabs

I did a quick test to see that it still works for keyboard only users and it seemed to be fine. We should do more testing with a screen reader though.

#185: I reported the same in #168, and in #169, LewisNyman said that that is out of scope for this issue.

Well, if this is out of the scope of this issue we might should postpone this one until the overlay one is in?

Actually, because the overlay tabs are a variation of the standard tabs, we have to postpone it until this patch is RTBC. All three patches need to be committed together.

Issue summary:View changes

Update summary to notify that overlay tabs are not supported in this issue.

I've added a test pages section to the summary of this issue, feel free to add some more pages. I'll produce some before/after screenshots when I get back on this issue.

Status:Needs work» Needs review

It looks like content header is RTBC again, lets make this happen asap?

Status:Needs work» Needs review
StatusFileSize
new49.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,514 pass(es), 0 fail(s), and 77 exception(s).
[ View ]
new32.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-192.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's the updated patch with the new patch from #2022695: Content header style update

StatusFileSize
new50.2 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
new33.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-195.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.58 KB

Looks like theme__menu_local_task shifted.

Status:Needs work» Reviewed & tested by the community

I am going to mark this RTBC, I tested it and it works (the combined patch + the overlay patch).

The dependencies and constant rerolling of patches makes this really difficult, so I hope it can get in without too much more hassle. This is one of the biggest style guide issues.

Please commit:
1) Content header style update
2) The patch news-tabs-1490402-195.patch
3) Implement Seven style guide for core overlay

Can the commit credit all designers (r5yn, Bojhan, yoroy) and developers involved?

Doesn't this still need accessibility and Javascript review? We have time to spare, #1953374: Implement Seven style guide for core overlay still needs work.

Status:Needs work» Needs review
StatusFileSize
new50.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,814 pass(es).
[ View ]
new33.25 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-203.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new789 bytes

Ok, I thought testbot was acting up but turns out it was that broken!

Here are some before/after shots:

Screen Shot 2013-10-28 at 18.40.10.pngScreen Shot 2013-10-28 at 18.41.33.pngScreen Shot 2013-10-28 at 18.40.22.pngScreen Shot 2013-10-28 at 18.41.23.png

  1. +++ b/core/includes/menu.inc
    @@ -2380,6 +2380,28 @@ function menu_local_tabs() {
    + * Returns a renderable element for the primary tabs.
    + */
    +function menu_local_primary_tabs() {
    +  $build = array(
    +    '#theme' => 'menu_local_tasks',
    +    '#primary' => menu_primary_local_tasks(),
    +  );
    +  return !empty($build['#primary']) || !empty($build['#secondary']) ? $build : array();
    +}
    +
    +/**
    + * Returns a renderable element for the secondary tabs.
    + */
    +function menu_local_secondary_tabs() {
    +  $build = array(
    +    '#theme' => 'menu_local_tasks',
    +    '#secondary' => menu_secondary_local_tasks(),
    +  );
    +  return !empty($build['#primary']) || !empty($build['#secondary']) ? $build : array();
    +}
    +++ b/core/themes/seven/seven.theme
    @@ -48,12 +49,84 @@ function seven_preprocess_html(&$variables) {
    +    $variables['secondary_local_tasks'] = array(
    +      '#theme' => 'menu_local_tasks',
    +      '#secondary' => isset($variables['tabs']['#secondary']) ? $variables['tabs']['#secondary'] : '',
    +    );

    Is all of this all needed? Seems odd to add new procedural functions to menu.inc while we try to delete them in other issues.

  2. +++ b/core/modules/system/css/system.module.css
    @@ -372,3 +372,60 @@ tr .ajax-progress-throbber .throbber {
    + * Remove browser styles, especially for <buttons> and so on.
    + */
    +.reset-appearance {
    +  -webkit-appearance: none;
    +  -moz-appearance:    none;
    +  appearance:         none;
    +  border: 0 none;
    +  background: transparent;
    +  padding: 0;
    +  margin: 0;
    +  line-height: inherit;
    +}
    +
    +/*
    + * Remove browser styles, especially for <buttons> and so on.
    + */
    +.reset-appearance {
    +  -webkit-appearance: none;
    +  -moz-appearance:    none;
    +  appearance:         none;
    +  border: 0 none;
    +  background: transparent;
    +  padding: 0;
    +  margin: 0;
    +  line-height: inherit;
    +}

    Duplicate?

  3. +++ b/core/modules/system/css/system.module.css
    @@ -372,3 +372,60 @@ tr .ajax-progress-throbber .throbber {
    +article,
    +aside,
    +details,
    +figcaption,
    +figure,
    +footer,
    +header,
    +hgroup,
    +main,
    +menu,
    +nav,
    +section,
    +summary {
    +  display: block;

    We don't have this anywhere else already?! Like, the html5shiv?

  4. +++ b/core/themes/seven/js/nav-tabs.js
    @@ -0,0 +1,20 @@
    +  attach: function (context, settings) {
    ...
    +    if ( tabs.length ) {

    Does this pass jsHint?

StatusFileSize
new52.21 KB
PASSED: [[SimpleTest]]: [MySQL] 59,364 pass(es).
[ View ]
new34.81 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-206.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.29 KB

@tim.plunkett nice catch on the system.module.css stuff. Looks like a bad re-roll. jshint didn't give me any complaints.

Is all of this all needed? Seems odd to add new procedural functions to menu.inc while we try to delete them in other issues.

I have no idea what do here, I just copied the existing function to do what I needed. Are there any issues that give a better direction?

I've also re-rolled this patch against the latest patch in #2022695: Content header style update which means breadcrumbs are back in.

Issue summary:View changes
Status:Needs review» Needs work
StatusFileSize
new6.28 KB
new3.53 KB
new11.64 KB

The tabs look very nice! A few nitpicks:

  1. Desktop: The use of border radius is inconsistent between the first and the other tabs (see: tabs-border-radius.png)
  2. Responsive mobile: border-top-radius is applied incorrectly to the last item (see: tabs-border-radius-responsive.png)
  3. Secondary tabs: active border shows above secondary navigation border instead of on the same line (see: tabs-secondary.png)

tim.plunkett's #205.4 remark is accurate: that's not according to Drupal's JS coding standards. JSHint doesn't check everything.

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
new32.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-210.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new50.18 KB
PASSED: [[SimpleTest]]: [MySQL] 60,260 pass(es).
[ View ]

Nice catches. It's hard to track all the minor changes over many iterations and re-rolls.

I have basically the same question as tim.plunkett: I don't see the two new function in menu.inc being used anywhere in the patch. What are they needed for?

Status:Needs work» Needs review
StatusFileSize
new1.9 KB
new32.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-213.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new49.88 KB
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es).
[ View ]

StatusFileSize
new7.98 KB

Made the JS way, way simpler.

Happy with it now.

I'm only posting the interdiff, apply on top of the latest patches. I'm don't want to mess about with the two patches (I made it on top of the combined content header and tab patch).

@nod_ Thanks soo much for helping out :) I am really happy to see this moving forward.

StatusFileSize
new31.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-219.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new49.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,072 pass(es).
[ View ]

Thanks _nod! I've rolled that into the other patches for review.

Status:Needs review» Needs work
StatusFileSize
new55.81 KB

I'm almost happy, secondary tabs are not yet ok.

Status:Needs work» Needs review
StatusFileSize
new3.12 KB
new31.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-223.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new50.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,212 pass(es).
[ View ]

It looks like some of nods Javascript changes included an assumption

Interdiff looks good to me. I didn't think of testing second tabs when I worked on it, sorry about that.

RTBC?

Issue tags:+alpha target

Would be great to get this into the next alpha, since the buttons patch went in. Tagging.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

This can't be RTBC if it fails without #2022695: Content header style update, which isn't RTBC, also for reasons below.

  1. +++ b/core/misc/jquery.collapse.js
    @@ -0,0 +1,98 @@
    +/**
    + * @file
    + */
    ...
    +;( function( $, window, document, undefined ) {
    ...
    +  function Collapse( element, options ) {
    +++ b/core/misc/jquery.dom-utils.js
    @@ -0,0 +1,59 @@
    + * @file
    + * Lowish-level DOM utilities for UI components
    + */
    +;( function( $, w, undefined ) {
    +++ b/core/misc/jquery.intrinsic.js
    @@ -0,0 +1,51 @@
    +/**
    + * @file
    + * Measure an element’s intrinsic width or height when neither constrained by
    + * a container nor forced full width as in 'display: block'.
    + */
    +(function ($) {
    ...
    +    display: 'table', /* 1 */
    +++ b/core/misc/jquery.nav-tabs.js
    @@ -0,0 +1,70 @@
    +/**
    + * @file
    + */
    +
    +;( function( $, window, document, undefined ) {

    Not our coding standard. Is this our file? If not, whose is it and where is the docblock explaining it?

    And what exactly related to the *styling of tabs* requires this much JS? There are no docs in these files saying what they do.

  2. +++ b/core/modules/shortcut/shortcut.module
    @@ -461,11 +461,14 @@ function shortcut_preprocess_page(&$variables) {
    -        '#title' => '<span class="icon">'. t('Add or remove shortcut') .'</span><span class="text">' . $link_text . '</span>',

    The "Add or remove shortcut" text is gone, is this accessible? I don't think this issue has had an accessibility review, even though it was asked for in #96, #180, and #200.

  3. +++ b/core/modules/system/css/system.theme.css
    @@ -438,15 +438,12 @@ ul.inline li {
    -.tabs > li {
    +.tabs__tab {

    This is just disgusting.

  4. +++ b/core/modules/views_ui/css/views_ui.admin.theme.css
    @@ -471,65 +481,66 @@ ul#views-display-menu-tabs li.add ul.action-list li{
       padding: 2px 9px;
    +
    }

    I think this was fine without the extra line.

  5. +++ b/core/themes/seven/seven.theme
    @@ -48,12 +62,82 @@ function seven_preprocess_html(&$variables) {
    +    drupal_add_library('seven', 'drupal.nav-tabs', FALSE);
    ...
    +    drupal_add_library('seven', 'drupal.nav-tabs', FALSE);

    Please do not add calls to drupal_add_* functions.

  6. +++ b/core/themes/seven/seven.theme
    @@ -48,12 +62,82 @@ function seven_preprocess_html(&$variables) {
    +    $variables['primary']['#prefix'] = '<h2 class="visually-hidden">' . t('Primary tabs') . '</h2>';
    +    $variables['primary']['#prefix'] .= '<nav role="navigation" class="is-horizontal is-collapsible" data-drupal-nav-tabs>';
    +    $variables['primary']['#prefix'] .= '<button class="reset-appearance tabs__tab tabs__trigger" data-drupal-nav-tabs-trigger>&bull;&bull;&bull;</button>';
    +    $variables['primary']['#prefix'] .= '<ul class="tabs primary clearfix"  data-drupal-nav-tabs-target>';
    +    $variables['primary']['#suffix'] = '</ul>';
    +    $variables['primary']['#suffix'] .= '</nav>';
    +    $output .= drupal_render($variables['primary']);
    ...
    +  return '<li' . (!empty($variables['element']['#active']) ? ' class="tabs__tab active"' : ' class="tabs__tab"') . '>' . $a_tag . '</li>';

    In one function we build up a render array, in the other we concatenate strings, and neither use twig. Why?

jquery.nav-tabs.js
jquery.collapse.js
jquery.dom-utils.js

are not needed, jquery.intrinsic.js is.

It looks great, but there are accessibility problems with this. There isn't any keyboard focus on the new tabs - admin/theme/install

There's no way for a keyboard only user to know when which tab is over focus.

Replicating the mouse over behavior should be fine. I haven't tested this with a screen reader yet.

Status:Needs work» Needs review
StatusFileSize
new43.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,415 pass(es).
[ View ]

Cheers for the feedback. I'll try and get round to this over the weekend.

In the meantime I've decided that keeping the patches in #2022695: Content header style update and this issue separate is more trouble than it's worth. Now that #1953374: Implement Seven style guide for core overlay is closed it doesn't actually help that much and makes it kind of confusing to contribute.

Have at it testbot.

Thanks for doing that, that makes it much easier to follow and make sure I'm reviewing/committing the right thing. :)

Tested this locally and overall it looks really great. MUCH "lighter" feel than the old header, and the tabs being on the left addresses a long-standing usability issue we've been finding since the D6 days and Garland.

Can't wait for this to get to RTBC! :D

+++ b/core/modules/shortcut/images/favstar.svg
@@ -0,0 +1,87 @@
+<?xml version="1.0" encoding="utf-8"?>
...
+<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px" width="80px"
+ height="20px" viewBox="0 0 80 20" enable-background="new 0 0 80 20" xml:space="preserve">
...
+<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">

Most of this info is extraneous, so if the SVG is relatively ready to go, it needs to be optimized. and could be stripped down to just <svg xmlns="http://www.w3.org/2000/svg" width="20" height="20">, just like chevron-left.svg and chevron-right.svg seen elsewhere in this patch.

Might be best to run it through svgo if possible.

StatusFileSize
new40.92 KB
PASSED: [[SimpleTest]]: [MySQL] 59,648 pass(es).
[ View ]
new20.17 KB

Here's a patch, I hope I caught everything. This was my todo list:

  • Minify SVG
  • Remove unnecessary JS files
  • Add keyboard focus
  • Check documentation for remaining JS files
  • Verify accessibility of shortcut text - I tested this in ChromeVox. It sounds fine to me.
  • Remove extra line in in view_ui.admin.theme.css
  • Remove calls to drupal_add_library
  • Sort out tabs theme function - I had a look at he original theme function in menu.inc and it does it the same way. This should be fixed but I'd rather not do it in this issue.
  • Tidy up CSS comments

I am happy to see that tim his feedback is addressed, I am going to mark this RTBC after #324 is fixed.

#324 ???

These tabs are such an improvement. Great work!

I have just a few comments, nothing major, but a few concern accessibility, so they're blocking RTBC, but they're nothing major.

  1. +++ b/core/modules/system/css/system.module.css
    @@ -349,3 +349,24 @@ tr .ajax-progress-throbber .throbber {
    +  -webkit-appearance: none;
    ...
    +  appearance:         none;
    ...
    +  -moz-appearance:    none;
    ...
    +  border: 0 none;

    Please remove the extra whitespace between the colons and the 'none'.

  2. +++ b/core/themes/seven/js/nav-tabs.js
    @@ -0,0 +1,56 @@
    + * Javascript behaviors the Seven tabs.

    This comment is awkward.

  3. +++ b/core/themes/seven/js/nav-tabs.js
    @@ -0,0 +1,56 @@
    +      var $tabs = $(context).find('[data-drupal-nav-tabs]');
    ...
    +        var notSmartPhone = window.matchMedia('(min-width: 300px)');

    This file needs to depend on the matchMedia polyfill if it doesn't already.

  4. +++ b/core/themes/seven/seven.theme
    @@ -27,6 +28,19 @@ function seven_library_info() {
    +      array('system', 'jquery.intrinsic'),

    Add system matchMedia as a dependency.

  5. +++ b/core/themes/seven/seven.theme
    @@ -48,12 +62,90 @@ function seven_preprocess_html(&$variables) {
    +    $variables['primary']['#prefix'] .= '<button class="reset-appearance tabs__tab tabs__trigger" data-drupal-nav-tabs-trigger>&bull;&bull;&bull;</button>';

    This text is read by a screenreader as "bullet bullet bullet". Add an aria-labelledby attribute with text that explains what this button is for.

  6. +++ b/core/themes/seven/seven.theme
    @@ -48,12 +62,90 @@ function seven_preprocess_html(&$variables) {
    +    $variables['primary']['#prefix'] .= '<nav role="navigation" class="is-horizontal is-collapsible" data-drupal-nav-tabs>';

    This needs an aria-labelledby attribute a reference to the H2 above it, which therefore needs an ID.

  7. +++ b/core/themes/seven/seven.theme
    @@ -48,12 +62,90 @@ function seven_preprocess_html(&$variables) {
    +    $variables['secondary']['#prefix'] .= '<nav role="navigation" class="is-horizontal" data-drupal-nav-tabs>';

    This needs an aria-labelledby attribute a reference to the H2 above it, which therefore needs an ID.

  8. +++ b/core/themes/seven/style.css
    @@ -255,152 +267,284 @@ pre {
    +  -webkit-box-sizing: content-box;
    +  -moz-box-sizing:    content-box;
    +  box-sizing:         content-box;

    Please remove extra whitespace.

StatusFileSize
new4.03 KB
new41.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-239.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks Jesse!

On your point 5; based on your phrasing and the documentation I read it seemed like the aria-label attribute was more appropriate for the trigger button. I assumed you mistyped. Correct me if I'm wrong.

The other fixes look good.

+++ b/core/modules/shortcut/shortcut.module
@@ -419,11 +419,14 @@ function shortcut_preprocess_page(&$variables) {
-        '#title' => '<span class="icon">'. t('Add or remove shortcut') .'</span><span class="text">' . $link_text . '</span>',
+        '#title' => '<span class="icon">'. $link_text .'</span>',

This was still not addressed.

I did go over it with a screen reader, it seems like redundant text? Why would you want to know that you can add or remove a shortcut when the next piece of text is actually "add shortcut" or "remove shortcut".

I did some hunting and found the line was added in this patch of #1217038: Clean up the CSS for the Shortcut module. It was never discussed as a requirement or directly commented on. It feels safe to drop here but I'd like a few more opinions.

Ok, so I applied the latest patch. Went here:
http://sca240513755191e.s3.simplytest.me/user/1/shortcuts

Searched for <span class="icon"> and am not seeing the duplication. I haven't been following this all that closely, but... @LewisNyman can you tell me how to replicate the redundant text?

      <header id="branding" class="clearfix">
    <div class="branding__inner">
              <h1 class="page-title">Shortcuts</h1>
            <div class="add-or-remove-shortcuts remove-shortcut"><a href="/admin/config/user-interface/shortcut/link/3/delete?link=user%2F1%2Fshortcuts&amp;name=admin&amp;destination=user%2F1%2Fshortcuts&amp;id=3" title="Remove from Default shortcuts"><span class="icon">Remove from <em class="placeholder">Default</em> shortcuts</span></a></div>
              <h2 id="primary-tabs-title" class="visually-hidden">Primary tabs</h2><nav role="navigation" class="is-horizontal is-collapsible" aria-labelledby="primary-tabs-title" data-drupal-nav-tabs><button class="reset-appearance tabs__tab tabs__trigger" aria-label="Primary tabs display toggle" data-drupal-nav-tabs-trigger>&bull;&bull;&bull;</button><ul class="tabs primary clearfix" data-drupal-nav-tabs-target><li class="tabs__tab"><a href="/user/1">View</a></li><li class="tabs__tab"><a href="/user/1/edit">Edit</a></li><li class="tabs__tab active"><a href="/user/1/shortcuts" class="active">Shortcuts<span class="visually-hidden">(active tab)</span></a></li></ul></nav>
          </div>
  </header>

I did some testing with VoiceOver, but think I'm searching in the wrong area for the redundant code.

I do appreciate though that the focus is now more obvious for keyboard only users. I'd still like it to be more visible, but it's alright. I like how the active tab's been managed.

@mgifford The duplication has been removed in this patch. If you test the shortcuts (not the tabs) in HEAD you'll hear the duplication.

Here's a comparison of the markup for the shortcuts in HEAD and in the latest patch.

HEAD

<div class="add-or-remove-shortcuts remove-shortcut">
  <a href="/admin/config/user-interface/shortcut/link/2/delete?link=admin%2Fcontent&amp;name=Content&amp;destination=admin%2Fcontent&amp;id=2">
    <span class="icon">Add or remove shortcut</span>
    <span class="text">Remove from <em class="placeholder">Default</em> shortcuts</span>
  </a>
</div>

Patch

<div class="add-or-remove-shortcuts remove-shortcut">
  <a href="/admin/config/user-interface/shortcut/link/2/delete?link=admin%2Fcontent&amp;name=Content&amp;destination=admin%2Fcontent&amp;id=2" title="Remove from Default shortcuts">
    <span class="icon">Remove from <em class="placeholder">Default</em> shortcuts</span>
  </a>
</div>

I agree in the the removal of the duplicate "Add or remove shortcut."

That's definitely an improvement for screen reader users.

However for keyboard only users I think there's a regression. When the star has focus there is no visible text to explain what the text/link does.

Using the title tag isn't good enough as that only provides additional context when you are hovering over it with a mouse.

I haven't tested this, but this might resolve the issue you raised while still giving the context to keyboard only users:

    <span class="icon"></span>
    <span class="text">Remove from <em class="placeholder">Default</em> shortcuts</span>

Yes that's true, I didn't think about that. I'd to figure a way to achieve this without cluttering the interface every time it's used.

StatusFileSize
new40.44 KB
FAILED: [[SimpleTest]]: [MySQL] 60,014 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Reroll.

I spoke to Bojhan on IRC, we decided that the best thing to do is re-implement the old helper text on hover/focus.

StatusFileSize
new3.75 KB
new41.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-250.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've added back the helper text to the shortcuts link on focus and hover. Only a few visual adjustments from the previous implementation.

Status:Needs review» Reviewed & tested by the community
Issue tags:-needs accessibility review

It looks like all the feedback has been addressed, including the a11y feedback.

Status:Reviewed & tested by the community» Needs review
Issue tags:+needs accessibility review

I don't see where we've actually done an accessibility review?

I think we resolved the bug, I can see it visually and it is announced by VoiceOver on Mac/Chrome.

#238 and #242

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new14.59 KB
new25.16 KB

Right, I'd seen the accessibility reviews earlier, but not since the latest patch. I ask specifically because the contrast does not seem to be good enough on secondary tabs. Compare the issue summary:

Black underline against a light grey line

With HEAD + this patch:

Black underline against a dark grey line

Maybe it's just my incredible near-sightedness but I absolutely can't pick out the active sub-tab in the second screenshot, like, at all. The colour of the "background line" seems too dark, and the "underline" line seems thinner to me than in the PSD, I dunno.

I realize this is a tiny detail in the grand scheme of the patch and probably just a simple color fix-up + a border tweak somewhere, but unfortunately not fixing it before commit would open a critical issue and we're not in the business of opening up critical issues for normal tasks. So if we could have one more re-roll to fix that, then I think we can call this good. Shortcuts are all working as expected (the design of the hover text clashes a bit, but that's just a normal follow-up to clean up), and I wasn't able to spot anything else from clicking around.

Hmm, interesting something changed in between patches - because it was different before. Good catch though, this will likely be a a11y regression. Not sure if it would actually classify as critical, but I am sure we can fix this. Since the original design is a11y valid on these contrast parts, because there is a shape change and a significant enough color change.

Status:Needs work» Needs review
StatusFileSize
new1.48 KB
new41.29 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch new-tabs-1490402-264.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Thanks guys, I sorted out the active/hover borders on secondary tabs.

I liked the one pixel width border for active/hover better. The problem was the tabs bottom border that was too dark gray I think.

Making active/hover border 2px width is more visible for sure but maybe this could be fixed just by making tabs bottom border lighter gray? Like the #260 mockup or even lighter.

@philipz I think we are better off sticking closer to the original designs for this issue and looking at a followup. We don't really have a process for updating the design in code and in the photoshop document together yet.

Status:Needs work» Needs review
StatusFileSize
new41.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,917 pass(es).
[ View ]

Re-roll. Who's feeling RTBC happy today? :)

Status:Needs review» Needs work
StatusFileSize
new34.24 KB

Tested on simplytest.me, when on narrow width where multiple tabs get reduced to one, I get no "..." menu, which usually appears in a second tab, as shown missing in screenshot. Also note the single tab's right corner is not rounded.

no-tab-menu.png

Nice catches. Though the secondary active tab is easy to pick out again from that screenshot, so hopefully we just do those touch-ups and then we're done. :)

Status:Needs work» Needs review
StatusFileSize
new1.7 KB
new41.33 KB
PASSED: [[SimpleTest]]: [MySQL] 63,360 pass(es).
[ View ]

Thanks Echoz. I think the previous patch applied strangely and chopped off a curly bracket.

Patch is all good for me, can't RTBC though. Hope it makes it :)

Seems to work well in my testing. Looks RTBC to me.

Status:Needs review» Reviewed & tested by the community

So nice! RTBC++

Adding related issue that will be fixed by this getting into Seven.

Awesome!! The bugs I found are now fixed, and it looks like echoz's are too.

The one thing that seems a bit weird to me is as you squish the window down it goes from this (all labels center-aligned, a tiny bit of padding around them):

Tabs, fully expanded.

To this (one massively wide tab with the label left-aligned):

One tab, collapsed.

However, given the styling underneath the "...", this seems like it's by design:

All tabs, expanded as a vertical list.

And in any event, we could always discuss tweaking this a bit more in a follow-up if it trips up other people. So this one is fine.

But unfortunately, I did find one regression in re-clicking around, which is in Views UI. :(

It goes from this:

One button marked 'Add'

to this:

A dummy blank space, + a dead button called 'Master', + the add button.

...which indicates a bug in either the CSS or JS. So I'm really sorry, but we should clean this up before commit, so we don't accidentally break any other pages in a more serious way.

This seems to be due to those display tabs rendering using
'#theme' => 'menu_local_task', from line 103 in core/modules/views_ui/lib/Drupal/views_ui/ViewFormControllerBase.php

And also the default seems to expect the tabs to be vertical if I'm reading that right it needs a .is-horizontal to not get that left border.

Not sure the right fix, if they should be treated like the rest of the tabs then they will need that .is-horizontal containing class and most of the views styles removed.

Status:Needs work» Needs review
StatusFileSize
new502 bytes
new41.38 KB
PASSED: [[SimpleTest]]: [MySQL] 63,404 pass(es).
[ View ]
new190.49 KB
new203.53 KB

So close!. Here's a patch that fixes the views regression, and some screenshots to confirm.

8.x
The views ui in 8.xWith patch
The views ui with patch applied

Happy to talk over the breakpoint, the tabs collapse when there is no space to fit the whole row, which means at some screen sizes there will be a rather big active tab. I'm not sure there is much we can do about that though.

Status:Needs review» Reviewed & tested by the community
Issue tags:+JavaScript

Works for me too.

Was this issue always moving the breadcrumbs? That's not in the issue summary at all, just making sure it didn't sneak in from another patch. I only noticed it in the last set of screenshots.

If that's purposeful, can it be added to the issue summary?

The views fix looks good, +1.

That change was from #2022695: Content header style update.
That was marked a duplicate of #1953374: Implement Seven style guide for core overlay.

Then that issue was marked as a duplicate of nothing, with an animated .gif as the only comment.

In #206 there was a mention of the breadcrumbs moving, but still in a separate issue.

I'm not opposed to the suggested change, but the issue title and summary and screenshots should reflect that we're doing more than redesigning the tabs.

Status:Needs work» Reviewed & tested by the community

testbot fluke

Title:Redesign tabs Redesign tabs and also move the breadcrumbs
Issue tags:-needs accessibility review+Needs issue summary update

The issue summary needs to acknowledge the actual scope of the issue, and the screenshots should not be cropped to exclude the breadcrumb changes.

Title:Redesign tabs and also move the breadcrumbsRedesign tabs and the content header
Issue summary:View changes
Related issues:+#2022695: Content header style update

Good point Tim. I've expanded the scope of the issue title further to include all the changes from #2022695: Content header style update and made this clear in the issue summary.

Status:Reviewed & tested by the community» Fixed

Awesome. I officially no longer have any complaints. YEAH!! :D

Committed and pushed to 8.x! Great work, everyone!

StatusFileSize
new35.34 KB

The moved breadcrumbs detach the right column in node edit forms :
https://drupal.org/files/issues/Node%20edit%20-%20breadcrumb.png

Issue summary:View changes

Fixing images in summary

Status:Fixed» Needs work

Happy to see this :-)

However. I have two serious/major complaints that I want to raise for this issue (and possibly other parallel Seven styleguide revamp issues):

  1. These changes must not touch system.base.css or system.theme.css.

    → Tabs are horribly broken in Stark right now.

    If you need to touch the base/default CSS in those files, then it is absolutely required to manually test all core themes.

    The changes to the base/default CSS files were inappropriate and wrong here, because the markup of the new CSS selectors only exists in the Seven theme. :-(

  2. This patch introduced massive changes + overrides for the Seven theme only.

    → None of this code can be leveraged by other admin themes.

    It's great to see that at least the jquery.intrinsic library has been split out. But aside from that, I wonder how sustainable changes like this are going to be in the long run? How much thought went into the topic of long-term maintenance?

    In almost all cases, themes provided by Drupal core should not have to override theme functions/templates in significant ways. If they have to, then that means that we've identified a generic problem with the default markup that is being produced by core.

    → Plenty of contrib and custom themes will have to copy/paste whichever one-off solution that was implemented in one of the core themes only.

    That is not sustainable. And that appears to be the case here: The Mobile Initiative is/was one of the major initiatives for D8. But yet, the meat of the solution for achieving responsive tabs is baked into a single, administrative theme only.

I have to re-open this issue at least for (1), because this change completely broke tabs in Stark, and Bartik looks quite a bit distorted, too.

Thanks for everyone's hard work here! Changes like this are certainly moving us in the right direction (disregarding the concerns I raised above) :-)

Status:Needs work» Needs review
StatusFileSize
new528 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,041 pass(es).
[ View ]

Attached patch reverts the faulty changes to system.theme.css of the committed patch.

@sun Could you document these principles somewhere? Because frankly, no one provides vision on how to deal with Stark. In your moments of absence, we then just push forward.

EDIT: On 2nd thought perhaps it also make sense to do this in a new issue?

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new98.94 KB
new100.42 KB

@sun Thanks. We don't have a firm process to test for visual regressions so mistakes like this have happened from time to time. #2099579: Discuss automated visual regression testing

Before:

After:

Let's make a new issue to make the tabs JS more reusable, some of the earlier patches were a lot like this. It was nod_'s decision to simplify the JS. We have discussed reusing the tab code already for Bartik.

themes provided by Drupal core should not have to override theme functions/templates in significant ways. If they have to, then that means that we've identified a generic problem with the default markup that is being produced by core.

I'm not completely sure I think everything we do in Seven has to be available for other themes, different designs require different solutions. As Seven becomes more “heavy handed” I think it's markup will move further away from core's default markup. That's an objective of dreammarkup.

Status:Reviewed & tested by the community» Fixed

Committed 33e3823 and pushed to 8.x. Thanks!

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.