More Seven Theme issues: #1986434: 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:
- 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).
- 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.
- 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
Related Issues
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:
- http://nav3.d8mux.org/structure/taxonomy/vocabulary/terms
- http://nav3.d8mux.org/structure/menus/menu
- http://nav3.d8mux.org/structure/content-types/type
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
Comment | File | Size | Author |
---|---|---|---|
#294 | drupal8.css-tabs.294.patch | 528 bytes | sun |
#290 | Node edit - breadcrumb.png | 35.34 KB | yched |
#280 | new-tabs-1490402-280.patch | 41.38 KB | LewisNyman |
Comments
Comment #1
JohnAlbinIf 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.
Comment #2
JohnAlbinTagging novice, as I think anyone with decent CSS skills can participate in the discussion and code it up.
Comment #3
LewisNyman CreditAttribution: LewisNyman commentedWe 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.
Comment #4
JohnAlbinFixing proper mobile-novice tag
Comment #5
JohnAlbinThe 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.
Comment #6
Ken Hawkins CreditAttribution: Ken Hawkins commentedI 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.
Comment #7
kika CreditAttribution: kika commentedWe 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.
Comment #8
Ken Hawkins CreditAttribution: Ken Hawkins commented@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:
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.
Comment #9
gmclelland CreditAttribution: gmclelland commented@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.
Comment #10
Bojhan CreditAttribution: Bojhan commentedComment #11
Ken Hawkins CreditAttribution: Ken Hawkins commented@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.
Comment #12
kika CreditAttribution: kika commented@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.
Comment #13
Bojhan CreditAttribution: Bojhan commentedI 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.
Comment #14
Bojhan CreditAttribution: Bojhan commentedReclassifying
Comment #16
RainbowArrayJust 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?
Comment #17
LewisNyman CreditAttribution: LewisNyman commentedI made a patch! Try out a demo on your phone here: tabs.d8mux.org
Comment #18
nod_This is awesome. I love it.
Comment #19
LewisNyman CreditAttribution: LewisNyman commentedBased 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:
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
Comment #20
LewisNyman CreditAttribution: LewisNyman commentedOops sorry, last patch sucked. This one rocks!
Comment #22
nod_Comment #23
nod_#20: drupal-mobile_tabs-1490402-20.patch queued for re-testing.
Comment #24
dasjo#17 is very cool. would love if we could have a simple solution like this
Comment #25
JohnAlbinIssue tags aren't Twitter hashtags. :-)
Comment #27
Shyamala CreditAttribution: Shyamala commented+ tag d8mux-admin
Comment #28
Shyamala CreditAttribution: Shyamala commented+ tag d8mux-admin
Comment #29
rteijeiro CreditAttribution: rteijeiro commentedHi.
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.
Comment #30
echoz CreditAttribution: echoz commentedNeeds 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!
Comment #31
vijay.cgs CreditAttribution: vijay.cgs commentedTested #20 patch. Now the scroll-bar helps to navigate all the links in menu.
Tested in Android 4.0 & IOS 6
Comment #32
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #33
vijay.cgs CreditAttribution: vijay.cgs commentedTested #32 path.
This patch also works well like #31
Comment #35
nod_looks good to me too :)
Comment #37
Dave ReidOdd behavior on Chrome 25 for Linux from http://tabs.d8mux.org/. Is this expected?
Comment #38
webchickThat looks like a needs work. :)
Comment #39
nod_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.
Comment #40
Dries CreditAttribution: Dries commentedThis 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.
Comment #41
webchickI 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.
Comment #42
webchicknod_ 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.
Comment #43
jessebeach CreditAttribution: jessebeach commentedRerolled to deal with a horizontal scrollbar on the page. This occurs because Seven declares
summary
elements to be width 100%. Butsystem.theme.css
givessummary
elements left/right padding. So they bust out of their containers. Thesummary
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.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
Comment #44
nod_That was solved in an other issue #1872606: Single Column admin pages, details summary causes scrollbar
Comment #45
jessebeach CreditAttribution: jessebeach commentedRighteous. 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.
Comment #46
danillonunes CreditAttribution: danillonunes commentedWhy can't we use the same pattern from Bartik's main menu?
Comment #47
LewisNyman CreditAttribution: LewisNyman commented@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.
Comment #48
Bojhan CreditAttribution: Bojhan commentedLets 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" :)
Comment #49
moshe weitzman CreditAttribution: moshe weitzman commentedIt 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.
Comment #50
webchickWe already did, don't worry. :) This is just a normal "needs work" to see if we can come up with something better.
Comment #51
vijaycs85Just a thought: gradient makes more sense, if we don't keep our tab style? and try something like below screenshot:
Or
Comment #52
ry5n CreditAttribution: ry5n commentedBojhan 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...
Comment #53
cweagans+1 for 52. That's beautiful.
Comment #54
dasjo#52 looks great
Comment #55
nod_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 :)
Comment #56
klonos...and here's a screenshot for the record:
Comment #57
LewisNyman CreditAttribution: LewisNyman commentedThe 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!
Comment #58
ry5n CreditAttribution: ry5n commented@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.
Comment #59
ry5n CreditAttribution: ry5n commentedSo… 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:
Comment #60
Bojhan CreditAttribution: Bojhan commentedThis direction feels fine, to me and much better than our current solution.
Comment #61
ry5n CreditAttribution: ry5n commented@Bojhan Cool. This was what was holding up the Overlay for patch, IIRC.
Comment #62
yoroy CreditAttribution: yoroy commentedOh my this looks and behaves very well! Who can help make the patch?
Comment #63
DamienMcKenna@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.
Comment #64
yoroy CreditAttribution: yoroy commentedNeeds work because needs patch. Thanks for testing DamienMcKenna.
Comment #65
ry5n CreditAttribution: ry5n commented@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.
Comment #66
nod_what about #1880488: Make menu collapsible on small screen resolutions ?
Comment #67
ry5n CreditAttribution: ry5n commentedLatest demo, with a fix for #63:
http://drupalcode.org/sandbox/ry5n/1932040.git/blob_plain/ec02de3c6a05ef...
Comment #68
rteijeiro CreditAttribution: rteijeiro commentedI 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.
Comment #69
ry5n CreditAttribution: ry5n commentedI’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.
Comment #70
tstoecklerWow, that demo is impressive!!! Awesome stuff.
Comment #71
rteijeiro CreditAttribution: rteijeiro commentedOk, now the problem in Firefox reported in #68 is solved. There are still some errors but is working really well. Great work!!
Comment #72
ry5n CreditAttribution: ry5n commented@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.
Comment #73
rteijeiro CreditAttribution: rteijeiro commentedSorry, here you are the error logs in Firefox 21 with Mac OS:
EDIT: put the log in a txt file in comment below.
Comment #74
klonos@rteijeiro: Hey Ruben! Can you please move the lengthy comment to a .txt file, upload the file and delete the comment. Thanx in advance.
Comment #75
rteijeiro CreditAttribution: rteijeiro commentedSure. Sorry for the mess... :)
Comment #76
klonosNo problem mate ;)
Thanx for keeping the issues clean. Long pages take time to load - especially in issues with lots of comments.
Comment #77
tkoleary CreditAttribution: tkoleary commentedtkoleary digitally high-fives ry5n.
Comment #78
ry5n CreditAttribution: ry5n commentedThanks 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.
Comment #79
LewisNyman CreditAttribution: LewisNyman commentedHere's the beginning, the no JS tabs.
Next step, write the Javascript to detect when there's enough room to display the menu horizontally.
Comment #80
LewisNyman CreditAttribution: LewisNyman commentedToday 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
Comment #81
ry5n CreditAttribution: ry5n commentedThanks 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.)
Comment #82
LewisNyman CreditAttribution: LewisNyman commentedNo 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.
Comment #83
nod_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 :)
Comment #84
LewisNyman CreditAttribution: LewisNyman commentedWith 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.
Comment #84.0
LewisNyman CreditAttribution: LewisNyman commentedAdded link to meta issue: #1870944
Comment #84.1
LewisNyman CreditAttribution: LewisNyman commentedUpdated issue summary to match the new title
Comment #85
LewisNyman CreditAttribution: LewisNyman commentedWe 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.
Comment #86
Bojhan CreditAttribution: Bojhan commentedCan this get a screeny? Last time I tried this it was quite buggy.
Comment #87
LewisNyman CreditAttribution: LewisNyman commentedComment #88
cweagansDO LIKE.
Great job on this :)
Comment #89
echoz CreditAttribution: echoz commentedI 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.
Comment #90
LewisNyman CreditAttribution: LewisNyman commentedHere'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.
Comment #91
Bojhan CreditAttribution: Bojhan commentedIt looks like this needs some design for the non-overlay styling. Ideally its part of the content header, not separate.
Comment #92
LewisNyman CreditAttribution: LewisNyman commented@Bojhan I've never seen a design that shows the tabs inside of the content header? See one of our sandbox examples
Comment #93
LewisNyman CreditAttribution: LewisNyman commentedOn Bojhan's request I tried moving that tabs into the content header, so they matched the same placement as the overlay design.
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.
Restricting the line to the width of the tabs seems a bit weird also.
Removing the line isn't an option.
Comment #94
LewisNyman CreditAttribution: LewisNyman commentedComment #96
tim.plunkettThere's no way this design meets WCAG 2.0
Comment #97
Bojhan CreditAttribution: Bojhan commentedLet 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.
Comment #98
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #99
tkoleary CreditAttribution: tkoleary commented@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.
Comment #100
tkoleary CreditAttribution: tkoleary commented@LewisNyman
Heres what views looks like:
Comment #101
tkoleary CreditAttribution: tkoleary commentedAnd here's another. Assuming I'm seeing > because the iconfont is not installed? Perhaps the character should be → ?
Comment #102
tkoleary CreditAttribution: tkoleary commentedAnd this:
Secondary tabs are larger than primary.
Comment #103
LewisNyman CreditAttribution: LewisNyman commentedThanks 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.
Comment #104
tim.plunkettAs long as it doesn't cause a visual regression, like #100 does.
Comment #106
LewisNyman CreditAttribution: LewisNyman commentedI 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 to is-activeComment #110
echoz CreditAttribution: echoz commentedI 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).
Comment #111
Bojhan CreditAttribution: Bojhan commentedSame here, tried to work on it today - but cant get it to apply at all.
Comment #112
LewisNyman CreditAttribution: LewisNyman commentedRe-rolled. There were a few commits that knocked this out.
Comment #115
Bojhan CreditAttribution: Bojhan commentedI think this is still failing, it doesnt apply at simplytest.me.
Comment #116
LewisNyman CreditAttribution: LewisNyman commentedRe-roll
Comment #117
jOksanen CreditAttribution: jOksanen commentedIm throwing my own stuff here as well. Many of the ones above look really good already.
Comment #118
tkoleary CreditAttribution: tkoleary commented@LewisNyman
Bog noted in #102 is still present in the latest patch.
Also the tabs still break at less than 320px.
Comment #119
LewisNyman CreditAttribution: LewisNyman commentedI 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
Comment #120
RainbowArrayOne 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.
Comment #121
LewisNyman CreditAttribution: LewisNyman commentedOk, it's worth giving it a go. We should to draw the line somewhere though, we can't make 100px widths look good.
Comment #122
yoroy CreditAttribution: yoroy commentedI'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).
Comment #123
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #125
aspilicious CreditAttribution: aspilicious commentedWhen 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.
Comment #126
LewisNyman CreditAttribution: LewisNyman commentedHm, 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.
Comment #127
aspilicious CreditAttribution: aspilicious commentedNo 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.
Comment #128
yoroy CreditAttribution: yoroy commentedNeeds work to fix the behaviour described in #125: Who's Online and Chatbox(https://drupal.org/node/1490402#comment-7780845)
Comment #129
Bojhan CreditAttribution: Bojhan commentedI will work on the visual design issue for now, tabs are not rounded - which is a problem.
Comment #130
Bojhan CreditAttribution: Bojhan commentedThere is a small overlay test bug, btw.
Comment #131
Bojhan CreditAttribution: Bojhan commentedReviewing 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
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
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.
Comment #132
Bojhan CreditAttribution: Bojhan commentedThis is ready for review and further work, I'd be nice to get this in an acceptable state before Prague.
Comment #134
RainbowArrayI'll try to run a reroll on this.
Comment #135
RainbowArrayRerolled this. Should work now. (There were two patches on #131 but they both look identical.)
Comment #137
LewisNyman CreditAttribution: LewisNyman commentedThe 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
Comment #139
rteijeiro CreditAttribution: rteijeiro commentedPatch 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.
Comment #140
LewisNyman CreditAttribution: LewisNyman commentedOk, 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.
Comment #141
echoz CreditAttribution: echoz commentedNice 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
Seventy-Eight
To test stacked secondary tabs, view Manage Display and enable a few more under Custom Display Settings until they stack on a narrow screen.
Comment #142
Bojhan CreditAttribution: Bojhan commented@echoz Could you hit me on IRC, I dont fully understand your concerns.
Comment #143
echoz CreditAttribution: echoz commented@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.
Comment #144
LewisNyman CreditAttribution: LewisNyman commented@echoz Nice catch, forgot about that, patch incoming.
Comment #145
LewisNyman CreditAttribution: LewisNyman commentedSorted out the secondary tabs and a few other things that came up.
Comment #148
LewisNyman CreditAttribution: LewisNyman commentedNow #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.
Comment #151
Bojhan CreditAttribution: Bojhan commentedIt still looks like its bugged. The bottom border should stretch the whole page.
Comment #152
Bojhan CreditAttribution: Bojhan commentedThis needs work, the border bottom is still wrong
Comment #153
LewisNyman CreditAttribution: LewisNyman commentedOk, 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.
Comment #156
LewisNyman CreditAttribution: LewisNyman commentedI think the patch will fail because it relies the content header patch. I will not apply just against HEAD. This still needs review
Comment #157
Bojhan CreditAttribution: Bojhan commentedWe should remove the 1px, 1px border width. The rest looks great now! :)
Before
After
Comment #158
mcjim CreditAttribution: mcjim commentedComment #159
mcjim CreditAttribution: mcjim commentedBorder-width changed as per #157
Haven't set to Needs Review as it depends on the content header patch, but it needs review.
Comment #162
yoroy CreditAttribution: yoroy commentedAh sorry, depends code-wise on the other one.
Comment #163
LewisNyman CreditAttribution: LewisNyman commentedHere's a combined patch of both applied together. Just so we can make sure we are passing tests.
Comment #164
aspilicious CreditAttribution: aspilicious commentedThe 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 :( :( :(
Comment #165
Bojhan CreditAttribution: Bojhan commentedYhea, 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?
Comment #166
LewisNyman CreditAttribution: LewisNyman commentedI'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.
I'm not sure what this means, can someone take a screenshot?
Comment #167
nod_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.
Comment #168
Wim LeersI was hoping to do in-depth manual testing, but the very first thing I tried was completely broken :(
Comment #169
LewisNyman CreditAttribution: LewisNyman commentedThis 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.
Comment #170
Wim LeersI found some minor problems, which I think are still important enough to require being fixed.
Also note that when moving from the first to the second screenshot, the buttons effectively become 1px taller! I doubt this is intentional?
Comment #170.0
Wim LeersA few tweaks
Comment #171
rteijeiro CreditAttribution: rteijeiro commented#169: @LewisNyman:
Issue summary updated ;)
Comment #172
nod_.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.
Comment #174
LewisNyman CreditAttribution: LewisNyman commentedOk, 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.
Comment #176
Wim LeersI 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.
Comment #177
LewisNyman CreditAttribution: LewisNyman commentedAck no, sorry I missed that on my screen.
Comment #178
tkoleary CreditAttribution: tkoleary commentedPatch 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.
Comment #179
tim.plunkettThis looks wrong.
Almost everywhere in core, and in our coding standards AFAIK, we don't have spaces after parens.
Same here, and throughout. Let's remove those please
Two lines.
Oh, I think I get it now. Is it to help when aggregating possibly malformed files?
Guessing you should update the RTL
/**
Please remove the extraneous spaces
Can someone post screenshots of these Views UI changes?
Initialize :)
Do we ever abuse prefix/suffix like this eleswhere? This looks pretty nutty.
only
*/
This and all other todos should be
@todo
, not@TODO:
, and should have issues opened for themComment #180
tim.plunkettAlso, this is still tagged "needs accessibility review", if that's done the tag can be removed, but this isn't RTBC if not.
Comment #181
Bojhan CreditAttribution: Bojhan commentedI asked the a11y team to chime in
Comment #182
Wim LeersWow, 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.
Comment #183
LewisNyman CreditAttribution: LewisNyman commentedThanks 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.
Comment #184
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #185
mgiffordI 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...
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.
Comment #186
Wim Leers#185: I reported the same in #168, and in #169, LewisNyman said that that is out of scope for this issue.
Comment #187
dawehnerWell, if this is out of the scope of this issue we might should postpone this one until the overlay one is in?
Comment #188
LewisNyman CreditAttribution: LewisNyman commentedActually, 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.
Comment #188.0
LewisNyman CreditAttribution: LewisNyman commentedUpdate summary to notify that overlay tabs are not supported in this issue.
Comment #189
LewisNyman CreditAttribution: LewisNyman commentedI'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.
Comment #190
Bojhan CreditAttribution: Bojhan commentedIt looks like content header is RTBC again, lets make this happen asap?
Comment #192
LewisNyman CreditAttribution: LewisNyman commentedHere's the updated patch with the new patch from #2022695: Content header style update
Comment #195
LewisNyman CreditAttribution: LewisNyman commentedLooks like theme__menu_local_task shifted.
Comment #197
Bojhan CreditAttribution: Bojhan commentedI 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?
Comment #200
LewisNyman CreditAttribution: LewisNyman commentedDoesn't this still need accessibility and Javascript review? We have time to spare, #1953374: Implement Seven style guide for core overlay still needs work.
Comment #203
LewisNyman CreditAttribution: LewisNyman commentedOk, I thought testbot was acting up but turns out it was that broken!
Comment #204
LewisNyman CreditAttribution: LewisNyman commentedHere are some before/after shots:
Comment #205
tim.plunkettIs all of this all needed? Seems odd to add new procedural functions to menu.inc while we try to delete them in other issues.
Duplicate?
We don't have this anywhere else already?! Like, the html5shiv?
Does this pass jsHint?
Comment #206
LewisNyman CreditAttribution: LewisNyman commented@tim.plunkett nice catch on the system.module.css stuff. Looks like a bad re-roll. jshint didn't give me any complaints.
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.
Comment #207
idebr CreditAttribution: idebr commentedThe tabs look very nice! A few nitpicks:
Comment #208
Wim Leerstim.plunkett's #205.4 remark is accurate: that's not according to Drupal's JS coding standards. JSHint doesn't check everything.
Comment #211
LewisNyman CreditAttribution: LewisNyman commentedNice catches. It's hard to track all the minor changes over many iterations and re-rolls.
Comment #213
tstoecklerI 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?
Comment #214
LewisNyman CreditAttribution: LewisNyman commentedComment #217
nod_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).
Comment #218
Bojhan CreditAttribution: Bojhan commented@nod_ Thanks soo much for helping out :) I am really happy to see this moving forward.
Comment #219
LewisNyman CreditAttribution: LewisNyman commentedThanks _nod! I've rolled that into the other patches for review.
Comment #222
aspilicious CreditAttribution: aspilicious commentedI'm almost happy, secondary tabs are not yet ok.
Comment #223
LewisNyman CreditAttribution: LewisNyman commentedIt looks like some of nods Javascript changes included an assumption
Comment #225
nod_Interdiff looks good to me. I didn't think of testing second tabs when I worked on it, sorry about that.
RTBC?
Comment #226
webchickWould be great to get this into the next alpha, since the buttons patch went in. Tagging.
Comment #227
Bojhan CreditAttribution: Bojhan commentedComment #228
tim.plunkettThis can't be RTBC if it fails without #2022695: Content header style update, which isn't RTBC, also for reasons below.
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.
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.
This is just disgusting.
I think this was fine without the extra line.
Please do not add calls to drupal_add_* functions.
In one function we build up a render array, in the other we concatenate strings, and neither use twig. Why?
Comment #229
nod_jquery.nav-tabs.js
jquery.collapse.js
jquery.dom-utils.js
are not needed, jquery.intrinsic.js is.
Comment #230
mgiffordIt 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.
Comment #231
LewisNyman CreditAttribution: LewisNyman commentedCheers 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.
Comment #233
webchickThanks 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
Comment #234
jwilson3Most 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.
Comment #235
LewisNyman CreditAttribution: LewisNyman commentedHere's a patch, I hope I caught everything. This was my todo list:
Comment #236
Bojhan CreditAttribution: Bojhan commentedI am happy to see that tim his feedback is addressed, I am going to mark this RTBC after #324 is fixed.
Comment #237
klonos#324 ???
Comment #238
jessebeach CreditAttribution: jessebeach commentedThese 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.
Please remove the extra whitespace between the colons and the 'none'.
This comment is awkward.
This file needs to depend on the matchMedia polyfill if it doesn't already.
Add system matchMedia as a dependency.
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.This needs an
aria-labelledby
attribute a reference to the H2 above it, which therefore needs an ID.This needs an
aria-labelledby
attribute a reference to the H2 above it, which therefore needs an ID.Please remove extra whitespace.
Comment #239
LewisNyman CreditAttribution: LewisNyman commentedThanks 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.Comment #240
tim.plunkettThe other fixes look good.
This was still not addressed.
Comment #241
LewisNyman CreditAttribution: LewisNyman commentedI 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.
Comment #242
mgiffordOk, 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?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.
Comment #243
LewisNyman CreditAttribution: LewisNyman commented@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
Patch
Comment #244
mgiffordI 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:
Comment #245
LewisNyman CreditAttribution: LewisNyman commentedYes 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.
Comment #248
LewisNyman CreditAttribution: LewisNyman commentedReroll.
I spoke to Bojhan on IRC, we decided that the best thing to do is re-implement the old helper text on hover/focus.
Comment #250
LewisNyman CreditAttribution: LewisNyman commentedI've added back the helper text to the shortcuts link on focus and hover. Only a few visual adjustments from the previous implementation.
Comment #255
Bojhan CreditAttribution: Bojhan commentedIt looks like all the feedback has been addressed, including the a11y feedback.
Comment #256
webchickI don't see where we've actually done an accessibility review?
Comment #257
Bojhan CreditAttribution: Bojhan commentedI think we resolved the bug, I can see it visually and it is announced by VoiceOver on Mac/Chrome.
Comment #258
LewisNyman CreditAttribution: LewisNyman commented#238 and #242
Comment #259
Bojhan CreditAttribution: Bojhan commentedComment #260
webchickRight, 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:
With HEAD + this patch:
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.
Comment #261
Bojhan CreditAttribution: Bojhan commentedHmm, 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.
Comment #264
LewisNyman CreditAttribution: LewisNyman commentedThanks guys, I sorted out the active/hover borders on secondary tabs.
Comment #265
philipz CreditAttribution: philipz commentedI 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.
Comment #268
LewisNyman CreditAttribution: LewisNyman commented@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.
Comment #269
LewisNyman CreditAttribution: LewisNyman commentedRe-roll. Who's feeling RTBC happy today? :)
Comment #270
echoz CreditAttribution: echoz commentedTested 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.
Comment #271
webchickNice 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. :)
Comment #272
LewisNyman CreditAttribution: LewisNyman commentedThanks Echoz. I think the previous patch applied strangely and chopped off a curly bracket.
Comment #273
nod_Patch is all good for me, can't RTBC though. Hope it makes it :)
Comment #274
mgiffordSeems to work well in my testing. Looks RTBC to me.
Comment #275
Bojhan CreditAttribution: Bojhan commentedComment #276
joelpittetSo nice! RTBC++
Comment #277
mgiffordAdding related issue that will be fixed by this getting into Seven.
Comment #278
webchickAwesome!! 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):
To this (one massively wide tab with the label left-aligned):
However, given the styling underneath the "...", this seems like it's by design:
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:
to this:
...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.
Comment #279
joelpittetThis 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.phpAnd 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.Comment #280
LewisNyman CreditAttribution: LewisNyman commentedSo close!. Here's a patch that fixes the views regression, and some screenshots to confirm.
8.x
With patch
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.
Comment #281
nod_Works for me too.
Comment #282
tim.plunkettWas 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.
Comment #283
tim.plunkettThat 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.
Comment #286
nod_testbot fluke
Comment #287
tim.plunkettThe issue summary needs to acknowledge the actual scope of the issue, and the screenshots should not be cropped to exclude the breadcrumb changes.
Comment #288
LewisNyman CreditAttribution: LewisNyman commentedGood 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.
Comment #289
webchickAwesome. I officially no longer have any complaints. YEAH!! :D
Committed and pushed to 8.x! Great work, everyone!
Comment #290
yched CreditAttribution: yched commentedThe moved breadcrumbs detach the right column in node edit forms :
https://drupal.org/files/issues/Node%20edit%20-%20breadcrumb.png
Comment #291
alexpottFixing images in summary
Comment #292
LewisNyman CreditAttribution: LewisNyman commentedFollow up created: #2195675: Breadcrumbs push the sidebar down on node edit pages
Comment #293
sunHappy 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):
These changes must not touch
system.base.css
orsystem.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. :-(
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) :-)
Comment #294
sunAttached patch reverts the faulty changes to
system.theme.css
of the committed patch.Comment #295
Bojhan CreditAttribution: Bojhan commented@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?
Comment #296
LewisNyman CreditAttribution: LewisNyman commented@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.
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.
Comment #297
LewisNyman CreditAttribution: LewisNyman commentedFollow up posted: #2207371: Abstract the Seven tabs functionality for reusability
Comment #298
alexpottCommitted 33e3823 and pushed to 8.x. Thanks!