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

Problem/Motivation

Buttons are an integral part of the admin UI experience, and currently there are many different styles of buttons used in core (dropbutton, manage display cog button, form buttons). None of these are aligned in style.

This has largely happened because we lacked of a styleguide for Drupal’s admin UI. Even though great, coherent UX work was done for Drupal 7, and we have functional UI standards, there was no central guide for the visual conventions used in Drupal’s core admin theme, Seven.

Since then, we developed Proposal: A Style Guide for Seven. This issue aims to introduce the proposed styling for buttons to core to create a unified and consistent look and behavior.

To quote the rationale provided:

Buttons should be clearly identifiable as such, with normal and primary actions inviting interaction. At the same time – for visual conciseness – graphic effects are limited to a subtle gradient (and border, where necessary for contrast with a background). For an informal, friendly appearance, identifiability among other UI controls, and for continuity with Drupal 7, discrete buttons have a fully rounded “pill” shape.

As with input fields, focus is communicated with a blue halo. The hover state brightens the button (optically, light colours ‘advance’) and adds a small shadow beneath, causing it to ‘stand up’ and invite a click. Primary buttons – the save button on a node form, for example – are emphasized using Drupal blue. This carries through the main brand colour, and, since blue is a calming colour, the added emphasis remains respectful, not yelling or rushing the action.

Delete buttons – those that perform a destructive action such as deleting a node – are colored red, since they must call attention and signal caution. However, they should not draw the eye away from more common actions, so their border and background is removed, appearing instead as a plain link. An underline provides an affordance, since danger buttons have neither the standard link colour nor the standard button style.

Note that all button colours pass WCAG 2.0, except for the primary button’s hover state, which warrants some consideration.

Proposed resolution

In #1945522: Add remaining buttons we laid the ground work for this change.

Screen Shot 2013-05-03 at 3.46.01 PM.png

We propose to change the font, change the styling to be more round and finally a new color revised color palette. Just the last two changes are part of this patch.

Remaining tasks

  • Make a patch
  • Review accessibility

User interface changes

The button style will be changed, no functional differences.

Test Pages

  • /core/install.php
  • /admin/content
  • /node/add/article
  • /admin/structure/views/view/content - Try changing a few settings
  • /admin/structure/views/add
  • /admin/structure/block - Try adding a block
  • /admin/structure/types
Files: 
CommentFileSizeAuthor
#114 Screen Shot 2013-11-22 at 4.14.59 PM.png12.44 KBwebchick
#109 seven_buttons_1986074_109.patch17.4 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 60,621 pass(es).
[ View ]
#98 interdiff.txt3.21 KBLewisNyman
#98 seven_buttons_1986074_98.patch17.19 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] 60,357 pass(es), 21 fail(s), and 0 exception(s).
[ View ]
#94 seven_buttons_1986074_94.patch17.22 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 59,806 pass(es).
[ View ]
#91 interdiff.txt1.98 KBLewisNyman
#91 seven_buttons_1986074_91.patch17.28 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 59,357 pass(es).
[ View ]
#89 interdiff.txt1.98 KBLewisNyman
#89 seven_buttons_1986074_89.patch533.45 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#83 performance-looks-good.png41.32 KBBarisW
#83 save-config-variant1.png27.26 KBBarisW
#83 save-config-variant2.png29.85 KBBarisW
#83 delete-node-not-red.png26.04 KBBarisW
#83 edit-node-three-styles.png41.43 KBBarisW
#82 seven_buttons_1986074_82.patch16.88 KBmcjim
FAILED: [[SimpleTest]]: [MySQL] 59,041 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#82 interdiff-seven_buttons_1986074_82.txt7.49 KBmcjim
#71 buttons.png121.04 KBrteijeiro
#68 seven_buttons_1986074_68.patch18.34 KBOuti
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]
#65 seven_buttons_1986074_65.patch18.32 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]
#65 Screen Shot 2013-09-21 at 17.40.12.png32.77 KBLewisNyman
#62 buttons-3-node-add-article.jpg31.42 KBOuti
#62 buttons-3b-node-1-edit.jpg69.37 KBOuti
#62 buttons-4-admin-structure-views-view-content.jpg77.34 KBOuti
#61 seven_buttons_1986074_61.patch18.41 KBOuti
PASSED: [[SimpleTest]]: [MySQL] 58,908 pass(es).
[ View ]
#57 interdiff.txt1.69 KBLewisNyman
#57 seven_buttons_1986074_56.patch18.4 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 seven_buttons_1986074_55.patch1.69 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#55 interdiff.txt18.4 KBLewisNyman
#52 seven_buttons_1986074_52.patch18.72 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 57,654 pass(es).
[ View ]
#47 seven_buttons_1986074_47.patch19.44 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 57,152 pass(es).
[ View ]
#43 seven_buttons_1986074_40.patch19.43 KBedward_or
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#40 local_task_d8_button_style.jpg47.04 KBmordonez
#39 Screen Shot 2013-06-28 at 15.03.22.png14.45 KBedward_or
#39 seven_buttons_1986074_39.patch19.43 KBedward_or
FAILED: [[SimpleTest]]: [MySQL] 58,731 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#38 seven_buttons_1986074_38.patch19.42 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]
#36 seven_buttons_1986074_36.patch19.46 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,176 pass(es).
[ View ]
#36 interdiff.txt3.32 KBLewisNyman
#35 Screen Shot 2013-06-21 at 10.16.45 AM.png11.07 KBtim.plunkett
#32 seven_buttons_1986074_31.patch19.22 KBLewisNyman
PASSED: [[SimpleTest]]: [MySQL] 58,014 pass(es).
[ View ]
#32 interdiff.txt3.77 KBLewisNyman
#29 seven_buttons_1986074_29.patch16.13 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] 58,630 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#29 interdiff.txt9.53 KBLewisNyman
#25 seven_buttons_1986074_25.patch13.8 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] 58,215 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#23 seven_buttons_1986074_23.patch13.42 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] 58,310 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#20 seven_buttons_1986074_20.patch13.65 KBLewisNyman
FAILED: [[SimpleTest]]: [MySQL] 57,672 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#5 seven_buttons_1986074_5.patch15.47 KBry5n
FAILED: [[SimpleTest]]: [MySQL] 55,654 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
Screen Shot 2013-05-03 at 3.46.01 PM.png104.24 KBBojhan

Comments

Bojhan’s picture

Issue tags:+Usability, +styleguide

Adding tags.

Bojhan’s picture

Issue summary:View changes

wtf

Bojhan’s picture

Issue summary:View changes

Updated issue summary.

Bojhan’s picture

Issue summary:View changes

Updated issue summary.

Bojhan’s picture

Issue summary:View changes

Updated issue summary.

Bojhan’s picture

Issue summary:View changes

Updated issue summary.

Bojhan’s picture

Issue summary:View changes

Updated issue summary.

ry5n’s picture

Assigned:Unassigned» ry5n

Patch incoming; give me two days.

oresh’s picture

r5yn, thanks for the buttons :)

also please take a loot at the button for search (from #1986418: Update textfield & textarea style);
I couldn't find any search without autosubmit, but we have different styles for that. Thank you.

ry5n’s picture

Assigned:ry5n» Unassigned
Status:Active» Needs work
StatusFileSize
new15.47 KB
FAILED: [[SimpleTest]]: [MySQL] 55,654 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

@oresh Thanks, I’ll look for that. For now I’ve just done the basic buttons. I’ve tried to apply the correct classes (class="link link--danger") for the danger buttons, using hook_element_info to define a new pre_render callback for button form elements, but that’s not working; can anyone help figure out why?

ry5n’s picture

Issue summary:View changes

tweakage

Bojhan’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_5.patch, failed testing.

Bojhan’s picture

@r5yn Not sure what you are trying to do here, nor do I think it makes much sense to sneak in the font here :P

ry5n’s picture

@Bojhan OK, here’s what the patch does, part-by-part:
1. Changes default classes for form buttons to the new class name standard
2. Adds the new Seven button CSS
3. Removes the existing Seven button styles (commented out for now, should be actually deleted)
4. Tries to alter the classes on delete buttons in Seven only, because Seven’s delete buttons are styled like links, so they need different classes. Rhis doesn't’t work for some reason
5. Styles depend on Source Sans, they will need tweaking with a different typeface, so Source sans is hacked in for testing. We need somebody to say yes or no on that before a final version of this patch can be rolled, and that would not include Source Sans. Not trying to sneak anything in.

Patch may need to touch Bartik to accommodate the change in CSS class conventions for buttons. At this point I just need help with the danger button classes.

Bojhan’s picture

@r5yn Not sure what you are trying to do with the danger button, it already has a class? Do you need something else?

ry5n’s picture

@Bojhan Yeah, they are not the right classes for Seven. Seven’s delete buttons are styled like links, so it doesn’t make sense to apply a button variant class that has to strip out all the button styles. Put another way, a class name describes an object’s appearance: it’s a label fot the way it looks. Since Seven’s danger buttons look like links, they need class="link link--danger", not button classes.

mcpuddin’s picture

Assigned:Unassigned» mcpuddin
mcpuddin’s picture

RE: hardcording Source Sans Pro

One of the people at todays con addressed #1986082: Add a webfont version of Source Sans Pro font, so if you apply that patch you can remove the hard-coding

RE: why hook_element_info_alter doesn't work

I noticed that you entered seven_element_info_alter to call seven_form_pre_render_button. I took a look at the invocation of hook_element_info_alter in common.inc and I noticed 2 things:

  • The modified "cache" is not returned because it is of type "managed_file" and not "button" ( which I don't think is the case )
  • The modified "cache" is not persistent long enough to get all the way to the actual invocation of pre_render

I believe the next steps are:

  1. Implement this correctly (unless is broken)
  2. Fix it in common.inc if its broken
  3. Find a different way to implement this modifications

I haven't had time to debug farther to see the specific issue but I'll take a look farther once I get a moment.

mcpuddin’s picture

I though it might be because hook_element_info_alter might be made only for a custom module not a theme ( not sure ? ) but I tried it in a custom module and it also didn't work properly

tim.plunkett’s picture

Note, this comments out code removed in the major bug #2003908: Regression of primary button styling

ry5n’s picture

@mcpuddin Thanks for looking into the hook_element_info_alter thing. I don’t really know where to start on #13 step 1. Any ideas? Really all it needs to do is change a couple of class names if the button #type is 'danger'.

@tim.plunkett Thanks, good to know. Those commented sections were intended to be deletions in the final patch.

mcpuddin’s picture

Assigned:mcpuddin» Unassigned
tim.plunkett’s picture

Issue tags:+Needs manual testing

The aforementioned regression was fixed.
This needs to be manually tested with a non-node entity form, preferably the Views UI.

ry5n’s picture

@tim.plunkett Thanks; also, yes for lots of manual testing. Before that though, I’m stuck on #5 (see #9.4 for more info). Any chance you could point me in the right direction?

ry5n’s picture

Issue summary:View changes

meta issue added

LewisNyman’s picture

StatusFileSize
new13.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,672 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

I got this patch to apply cleanly again, my D8 install is borked so I wasn't able to look into the hook_element_info stuff.

LewisNyman’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_20.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new13.42 KB
FAILED: [[SimpleTest]]: [MySQL] 58,310 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

Re-roll

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_23.patch, failed testing.

LewisNyman’s picture

StatusFileSize
new13.8 KB
FAILED: [[SimpleTest]]: [MySQL] 58,215 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

I've taken a different approach with this patch.

We were being a bit anal about mark up and CSS. There is a time and a place for that and this is not it. I've added the button--danger class so it resets the base button class styling. Not optimal but it keeps an issue moving that has otherwise stalled.

I've modified the installer forms as an example: They add the button type semantics, they display as primary buttons. I'm not sure whether we should aim to add the correct button type semantics to all forms or leave that to follow up issues?

LewisNyman’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_25.patch, failed testing.

Bojhan’s picture

@Lewis I would split that off as much as possible. That might need a little more discussion as it should be done all over Core. Not just Seven.

LewisNyman’s picture

StatusFileSize
new9.53 KB
new16.13 KB
FAILED: [[SimpleTest]]: [MySQL] 58,630 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

With that in mind, I've focused purely on getting the CSS up to scratch. I've removed some conflicting CSS and patched up some specificity related errors.

LewisNyman’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_29.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new3.77 KB
new19.22 KB
PASSED: [[SimpleTest]]: [MySQL] 58,014 pass(es).
[ View ]

Looks like I need to update some tests for this to pass. I've never had to do that in a patch before.

Bojhan’s picture

I looked around core and noticed all the different buttons, it looks pretty nice - including the danger button. I personally dislike the "pressed" state which shows an inverted gradient. But I am OK with fixing that later.

Small action button - there seems to be not enough letter spacing/tracking making illegible.

I'd say the issue above is the only thing that needs to be fixed here still.

Bojhan’s picture

Issue tags:+Usability

Oh tags.

tim.plunkett’s picture

StatusFileSize
new11.07 KB

I think my only major complaint is the reduced line height on local actions ("Add link" in this picture)
Screen Shot 2013-06-21 at 10.16.45 AM.png

I still think the overuse of rounded corners is silly, but that line height is the only blocker I can see.

LewisNyman’s picture

StatusFileSize
new3.32 KB
new19.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,176 pass(es).
[ View ]

A little bit of work tweaking button--small. I've added 1 pixel of vertical padding either side. I looks a bit beter now but it no longer matches the height of form elements, which was quite nice.

I also removed the inverted gradient on press. I didn't like it either, just the inset drop shadow is nicer. I thought it was an accessibility thing.

Bojhan’s picture

@Lewis It kinda was an a11y thing, but frankly I am not sure if it needs to be there. I hope that a11y people at some point will chime in, sadly they haven't been providing much feedback yet.

Could you show the tweaked action button? Lets keep in mind though that we designed it to be smaller than an ordinary button. Because if its as big as a button, it is distracting and voluptuous which is at odds with our more tone-down styled. It also looks weird when you have two buttons near each other, as they don't differentiate.

@tim I understand you don't like the roundness. I hope you our arguments in the https://groups.drupal.org/node/283223 explain our position from the visual design side, with a style change like this its really hard to please everyone - as different tastes exist, its hard to find one that pleases everyone . Our goal is to make a consistent but strong style, with that we will introduce elements that not everyone likes. But in the end it will hopefully strengthen the brand, we fear that trying to please everyone will instead create a very broad style guide - which will be far less effective.

I am very open to discussing this more, as I am on vacation its a little hard - feel free to ping me on IRC about this, once I am around, I wanna make sure we are not upsetting anyone with this.

LewisNyman’s picture

StatusFileSize
new19.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,418 pass(es).
[ View ]

Re-roll

edward_or’s picture

StatusFileSize
new19.43 KB
FAILED: [[SimpleTest]]: [MySQL] 58,731 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new14.45 KB

Looks great. Few questions

1. Should we wrap text in a button if text is larger then width? Currently text doesn't wrap. See screenshot.
2. Should we set explicitly set .button to not be underlined as a default? If this class was to be useable on a link tag for example.

Also noticed that .button--primary had its text-shadow set with rgba and not hsla. Also not sure if intentional or not it had 1px set as the y-offset which looked a little worse to me.

mordonez’s picture

StatusFileSize
new47.04 KB

I've tested the new button for local actions and looks like the image.

button local task

mordonez’s picture

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch, seven_buttons_1986074_39.patch, failed testing.

edward_or’s picture

StatusFileSize
new19.43 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_40.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Trying again with a more recent version of 8.x

edward_or’s picture

Status:Needs work» Needs review
LewisNyman’s picture

Status:Needs review» Needs work
Issue tags:+Usability, +Needs manual testing, +needs accessibility review, +styleguide

The last submitted patch, seven_buttons_1986074_40.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new19.44 KB
PASSED: [[SimpleTest]]: [MySQL] 57,152 pass(es).
[ View ]

Rerolled

fubhy’s picture

This is my application for the most nitpicky review ever. (period)

+++ b/core/themes/seven/css/components/buttons.cssundefined
@@ -0,0 +1,70 @@
+ * Structural styles for Seven’s UI buttons

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+ * Stylistic treatment for Seven’s UI buttons

Oh I hate myself for saying this, but please use ' instead of `

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+ * 2. Prevent fat text in WebKit

Missing period.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+ * Overrides styling from system.theme

Missing period.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+/*.boxshadow*/ .button:focus {
...
+/*.boxshadow*/ .button:active:focus,
+/*.boxshadow*/ .button.is-active:focus {

.boxshadow is commented out. Is that correct?

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+/* We've temporaily added the danger button here, bit of a harsh reset but we need it */

Typo in 'temporaily'. Also, 80 characters.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,244 @@
+/* Compensate for all buttons pulled left by 1px */

Missing period.

+++ b/core/themes/seven/seven.themeundefined
@@ -141,6 +144,44 @@ function seven_tablesort_indicator($variables) {
+  // Will be the existing classes array or NULL
...
+  // We require modernizr for button styling

Missing period.

+++ b/core/themes/seven/seven.themeundefined
@@ -141,6 +144,44 @@ function seven_tablesort_indicator($variables) {
+  $classes = array_merge((array)$classes, array('button', 'button--primary', 'button--small'));

Missing whitespace after the typecast.

+++ b/core/themes/seven/seven.themeundefined
@@ -141,6 +144,44 @@ function seven_tablesort_indicator($variables) {
+  drupal_add_library('system', 'modernizr');

Can we move these to #attached somewhere?

+++ b/core/themes/seven/seven.themeundefined
@@ -141,6 +144,44 @@ function seven_tablesort_indicator($variables) {
+  // We require modernizr for button styling

Missing period.

oresh’s picture

Status:Needs review» Needs work

@fubhy - you're a period lover, aren't you? :)
/*.boxshadow*/ - @LewisNyman please verify. either remove the class or remove the comments :) Don't leave a commented selector in css. Thank you!

ry5n’s picture

Those commented .boxshadow selectors are there because I wanted to only override focus outlines in browsers that can render the alternative – otherwise it’s an a11y issue. However, the boxshadow test was deemed too expensive to add to core’s version of Modernizr, so the styles are commented out. We should be able to use a conditional class or something to restore outline for IE8 at least.

LewisNyman’s picture

I think, for the sake of getting the new styling in, we should postpone the focus styling to a follow up issue.

LewisNyman’s picture

Status:Needs work» Needs review
Issue tags:-Needs manual testing
StatusFileSize
new18.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,654 pass(es).
[ View ]

I have removed the focus styling and implemented all the changes in #48 apart from:

+++ b/core/themes/seven/seven.themeundefined
@@ -141,6 +144,44 @@ function seven_tablesort_indicator($variables) {
+  drupal_add_library('system', 'modernizr');

Based on the conversation in #1969916: Remove drupal_add_js/css from seven.theme I don't think it's feasible to implement #attached right now. I could be wrong in this instance though.

Bojhan’s picture

Assigned:Unassigned» Wim Leers

@Wim Could you provide feedback on this, how to handle the attach issue.

LewisNyman’s picture

Assigned:Wim Leers» Unassigned

Looks like there's an answer here.

LewisNyman’s picture

StatusFileSize
new18.4 KB
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've added the same technique used in #1969916: Remove drupal_add_js/css from seven.theme. I also removed the outline from the hover/focus styles, I'm not sure if we can get away with just the current focus styles alone.

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_55.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new18.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_56.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.69 KB

Whoops! I mixed up the file names.

tstoeckler’s picture

+++ b/core/themes/seven/seven.theme
@@ -178,7 +175,14 @@ function seven_menu_local_action($variables) {
-  drupal_add_library('system', 'modernizr');
+    $libraries = array(
+    '#attached' => array(

Wrong indentation here.

Leaving at needs review for actual code review.

tstoeckler’s picture

Issue summary:View changes

Updated issue summary.

Outi’s picture

Status:Needs review» Needs work
Issue tags:+Usability, +needs accessibility review, +styleguide

The last submitted patch, seven_buttons_1986074_56.patch, failed testing.

Outi’s picture

Status:Needs work» Needs review
StatusFileSize
new18.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,908 pass(es).
[ View ]

I rewrote the patch as the latest one didn't apply on the actual state.

Outi’s picture

Status:Needs review» Needs work
StatusFileSize
new77.34 KB
new69.37 KB
new31.42 KB
new42.74 KB

Some screenshots of the buttons with the latest patch on the test pages:

/admin/content
/node/add/article
/node/1/edit
/admin/structure/views/view/content

LewisNyman’s picture

Status:Needs work» Needs review
Bojhan’s picture

Hmmm, it looks like the buttons are a bit big - especially their font-size.

LewisNyman’s picture

StatusFileSize
new32.77 KB
new18.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,542 pass(es).
[ View ]

I think the previous font sizing was based on Source Sans Pro. I've reduced the font size by one pixel to 14px. 13px looks too small.
Screen Shot 2013-09-21 at 17.40.12.png

Bojhan’s picture

There we go, that looks much better :). The font weight is incorrectly applied to the danger styling, that should not have a 600 point weight, it should have no additional weight.

LewisNyman’s picture

Status:Needs review» Needs work
Outi’s picture

StatusFileSize
new18.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]

Sets the danger button font-weight at 400.

Outi’s picture

Status:Needs work» Needs review
Bojhan’s picture

Yup, this looks correct! Thanks

rteijeiro’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new121.04 KB

Patch applies well and danger button size looks correct. Let's go for RTBC!

Danger button

LewisNyman’s picture

I've tagged this issue with “styleguide-independent”, which means it can be committed on it's own without causing visual regressions.

nod_’s picture

I'm concerned about this patch use of drupal_add_library and #attached, need wim, sdboyer or catch to double check.

Bojhan’s picture

Assigned:Unassigned» Wim Leers
Status:Reviewed & tested by the community» Needs review

Lets see if Wim can help us

nod_’s picture

Status:Needs review» Reviewed & tested by the community

Checked with sam on IRC, all good for now.

sdboyer’s picture

Checked with sam on IRC, all good for now.

yeah, lemme just qualify that a tad. in the hard problems discussion of render caching/drupal_render(), Fabianx and catch suggested a refactoring of drupal_render() that *should* allow this sort of thing to be OK...but that remains theory. as things currently stand, however, adding libraries in #attached while within preprocess or theme functions would be breaky for our attempts to rip out drupal_add_css() and drupal_add_js() globals.

i talked with catch about it briefly, and he didn't have a gut feeling on whether or not this should go forward. my feeling is that *actually* killing all the globals is still a little ways away, and since we already have instances of these sorts of things elsewhere, i won't un-RTBC. i will, however, say that if there IS another not-terribly-difficult way of moving the library attaching into somewhere that's neither a theme function nor a preprocess, then please, do that :)

so yes, i hereby vow to eat shit if we end up not being able to accommodate library declarations in theme/preprocess functions.

Wim Leers’s picture

Assigned:Wim Leers» Unassigned
Status:Reviewed & tested by the community» Needs work
Issue tags:+CSS

I'm sorry, lots of small problems here … :) But looking very nice, and LOVING the explanations for the "specialty CSS"! But, really, I think this should be reviewed by a CSS expert who hasn't worked on this issue yet (maybe John Albin?) — I'm not the right person to review this. I think you're almost there though, and can't wait to see this improvements land to Drupal :)

  1. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,70 @@
    +/**
    + * 1. Enable z-index on buttons.
    + * 2. Normalize 'line-height'; can’t be changed from 'normal' in Firefox 4+.
    + * 3. Allows full range of styling in Webkit and Gecko.
    + *
    + * N.B. Assumes box-sizing: border-box applied globally.

    This is AWESOME. Thank you.

  2. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,70 @@
    +  /* @TODO Move into base.css under a universal selector (*) */

    s/@TODO/@todo/

    Why can't this be done yet in this patch?

  3. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,70 @@
    +  /*z-index: 10;*/

    Commented CSS? Should be removed.

  4. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,70 @@
    +/* ====================================
    +   Link actions
    +   ==================================== */
    ...
    +/* ====================================
    +   Button groups
    +   ==================================== */
    +

    +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +/* ====================================
    +   Buttons
    +   ==================================== */
    +

    I haven't seen this pattern before in Drupal core. Is this acceptable?

  5. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,70 @@
    +}
    +
    +.button-group > .button {

    Why a newline here, but not above?

  6. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  font-size: 1.076923077em;

    WTF?

  7. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  text-decoration: none;

    Why repeat this if it's already set on .button?

  8. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  -webkit-transition: none;
    +  -moz-transition:    none;
    +  -o-transition:      none;
    +  transition:         none;
    +}

    Why not apply the similar nice whitespace indentation thing that is applied to the linear gradient background CSS?

  9. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  border-color: #1e5c90;

    Same as .button--primary — why repeat?

  10. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  font-size: 14px;
    +  line-height: 16px;

    I'm not a CSS expert, but it feels weird to be using px sizes for this rather than relative sizes? If there's good reasons for this, just answer "for good reasons, you CSS n00b", and forget about this :)

  11. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  /* @TODO uncomment once base typography from the style guide is in - #1986082 */
    +  /*font-size: 0.813rem;*/

    Another unresolved todo that looks like it could be resolved.

  12. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  color: #0074bd; /* hsl(203, 100%, 37%) */
    ...
    +  color: #008ee6; /* hsl(203, 100%, 45%) */

    Comments that should be deleted.

  13. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    + * We've temporarily added the danger button here, bit of a harsh reset
    + * but we need it

    80 col wrapping

  14. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  padding: 0!important;
    +  border: 0!important;
    ...
    +  box-shadow: none!important;
    +  background: none!important;

    !important — WAT?

  15. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,225 @@
    +  text-decoration: underline;
    +  text-shadow: none;
    ...
    + text-decoration: none;

    Already set in .button--danger.

  16. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +  // Will be the existing classes array or NULL

    Missing trailing period.

  17. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +  // We require modernizr for button styling.

    s/modernizr/Modernizr/

  18. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +    $libraries = array(

    Wrong indentation.

  19. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +  // @TODO Replace with a generalized solution for icons.

    s/@TODO/@todo/

  20. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +function seven_element_info_alter(&$types) {

    s/$types/$type/

    (As per the other hook implementations.)

  21. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +  // We require modernizr for button styling

    - s/modernizr/Modernizr/
    - Missing trailing period.

  22. +++ b/core/themes/seven/seven.theme
    @@ -163,6 +163,51 @@ function seven_tablesort_indicator($variables) {
    +    drupal_add_library('system', 'modernizr');

    1) Why do we have this if we're attaching it in the preceding hook implementation anyway?

    2) This should be changed to

    $types['button']['#attached']['library'][] = array('system', 'modernizr');
Xano’s picture

Some of those code comments that Wim quoted contain contractions or abbreviations. We should only use full words for readability.

ry5n’s picture

+++ b/core/themes/seven/css/components/buttons.cssundefined
@@ -0,0 +1,70 @@
+  /* @TODO Move into base.css under a universal selector (*) */
+  -webkit-box-sizing: border-box;
+  -moz-box-sizing: border-box;
+  box-sizing: border-box;

Note it’s the box-sizing rule only that should be moved to Seven’s base css.

+++ b/core/themes/seven/css/components/buttons.cssundefined
@@ -0,0 +1,70 @@
+/* Prevent focus ring being covered by next siblings. */
+.button:focus {
+  /*z-index: 10;*/

Can be removed if we are dropping the custom focus styles. Otherwise should be uncommented and retained.

+++ b/core/themes/seven/css/components/buttons.cssundefined
@@ -0,0 +1,70 @@
+/* ====================================
+   Link actions
+   ==================================== */

These are from my original work; this is my preferred sectioning style but does not conform to Drupal standards. Can be removed, although it makes the stylesheet less readable.

+++ b/core/themes/seven/css/components/buttons.cssundefined
@@ -0,0 +1,70 @@
+}
+
+.button-group > .button {

Stray newline can be removed.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  font-size: 14px;
+  font-size: 1.076923077em;

This should be fixed to use either em or (preferably) rem units. We can use rems since IE8 is no longer supported, and they provide global scalability while ensuring that buttons won’t grow or shrink in size when placed in sections with different font sizes. Unless that is the desired effect (in that case, use ems).

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  text-decoration: none;

`text-decoration` repeated on all states as a specificity precaution – it’s likely to be specified elsewhere on hover, e.g. a:hover. The less desirable alternative is !important.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  -webkit-transition: none;
+  -moz-transition:    none;
+  -o-transition:      none;
+  transition:         none;

`linear-gradient` prefixed rules are indented so their values align (making multiline editing easier). This follows the same pattern.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  border-color: #1e5c90;

Since `.button--primary` is applied always to `.button` elements, we need to ensure that the primary’s border color doesn’t get overridden by the base hover state.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  font-size: 14px;

Should be a relative unit, em or rem.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  font-size: 13px;

Same here.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+  font-size: 13px;
+  /* @TODO uncomment once base typography from the style guide is in - #1986082 */
+  /*font-size: 0.813rem;*/

And here, etc.

+++ b/core/themes/seven/css/components/buttons.theme.cssundefined
@@ -0,0 +1,225 @@
+.button--danger {

These appear visually as links, not buttons, so they should get the .link class and extend it as .link--danger. This lets us avoid resetting the hell out of the button styles. The tricky part is to apply the right classes on the right elements. This is harder than it should be :)

ry5n’s picture

BTW (and maybe @LewisNyman can answer this) do we have anything using the `button-group` styles in yet? Otherwise, maybe they can be moved into the patches for the issues that need them?

LewisNyman’s picture

Yep, we should move any button-group styling to #1989470: Dropbutton style update for Seven

mcjim’s picture

Status:Needs work» Needs review
StatusFileSize
new7.49 KB
new16.88 KB
FAILED: [[SimpleTest]]: [MySQL] 59,041 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Incorporated comments from #77 and #79.

Removed button-group styles for now as per #80.

BarisW’s picture

I believe this needs a little bit more love.

Some screenshots.

Performance page: good.
performance-looks-good.png

Save theme config: variant 1
save-config-variant1.png

Save theme config: variant 2
save-config-variant2.png

Delete a node: no red button / link?
delete-node-not-red.png

Edit node: do we need three different styles?
edit-node-three-styles.png

Bojhan’s picture

The Save theme config: variant 1 should ideally be fixed. There is no reason not to have a general Save button below the fieldset, it being in a fieldset is a little wierd.

So I think all these delete button issues are separate. We have different styling applied to different places, this should all be made consistent. I am not sure if that is part of this patch, but it would be great to fix.

The save and keep published has a new design, I assume once this is in we work on that.

LewisNyman’s picture

The save and keep published design will be tackled in #1989470: Dropbutton style update for Seven

There are loads of places where we are not using the correct button type. Let's break those down in other issues though.

nod_’s picture

#82: seven_buttons_1986074_82.patch queued for re-testing.

nod_’s picture

Status:Needs review» Reviewed & tested by the community

Should apply fine, let's get that in and work on the follow-ups :)

Status:Reviewed & tested by the community» Needs work

The last submitted patch, seven_buttons_1986074_82.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new533.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch seven_buttons_1986074_89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.98 KB

Looks like theme_menu_local_action shifted. Good catch!

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_89.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new17.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,357 pass(es).
[ View ]
new1.98 KB

My mistake, messed up my branches.

Bojhan’s picture

Ok, RTBC :) Please credit the designers.

Committers keep in mind that bugs like noted in #83 will be there, they are however not related to this patch

tim.plunkett’s picture

Status:Needs review» Needs work
  1. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,46 @@
    +}
    \ No newline at end of file

    .

  2. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,179 @@
    +}
    \ No newline at end of file

    .

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new17.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,806 pass(es).
[ View ]

Thanks.

Bojhan’s picture

Status:Needs review» Reviewed & tested by the community
nod_’s picture

#94: seven_buttons_1986074_94.patch queued for re-testing.

Wim Leers’s picture

Status:Reviewed & tested by the community» Needs work

Sorry to spoil the party, but this still isn't ready yet.

  1. +++ b/core/themes/seven/css/components/buttons.css
    @@ -0,0 +1,46 @@
    +  /* @todo Move box-sizing into base.css under a universal selector (*) */

    Why is this still a @todo?

    (I asked that in ##7, on Sep 25 already!)

  2. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,179 @@
    +  font-size: 14px;
    +  font-size: 1.076923077em;

    From ry5n's comment in #79:

    This should be fixed to use either em or (preferably) rem units. We can use rems since IE8 is no longer supported, and they provide global scalability while ensuring that buttons won’t grow or shrink in size when placed in sections with different font sizes. Unless that is the desired effect (in that case, use ems).

  3. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,179 @@
    + * @todo replace with link--danger.
    + * See https://drupal.org/node/1986074#comment-7895913.

    Why can't we do this here yet?

  4. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -0,0 +1,179 @@
    +  padding: 0!important;
    +  border: 0!important;
    ...
    +  box-shadow: none!important;
    +  background: none!important;

    That's a whole lot of !important

  5. +++ b/core/themes/seven/seven.theme
    @@ -158,6 +158,59 @@ function seven_tablesort_indicator($variables) {
    +    // We require Modernizr for button styling.
    +  $libraries = array(

    Spacing is off.

  6. +++ b/core/themes/seven/seven.theme
    @@ -158,6 +158,59 @@ function seven_tablesort_indicator($variables) {
    +    $output .= Drupal::l($link['title'], $link['route_name'], $link['route_parameters'], $link['localized_options']);
    ...
    +    $output .= l($link['title'], $link['href'], $link['localized_options']);

    \Drupal::l()

  7. +++ b/core/themes/seven/seven.theme
    @@ -158,6 +158,59 @@ function seven_tablesort_indicator($variables) {
    +  // We require Modernizr for button styling.

    Because…?

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new17.19 KB
FAILED: [[SimpleTest]]: [MySQL] 60,357 pass(es), 21 fail(s), and 0 exception(s).
[ View ]
new3.21 KB

Thanks for the review Wim.

I've address all the issues mentioned above apart from the two @todos:

+++ b/core/themes/seven/css/components/buttons.css<br>@@ -0,0 +1,46 @@<br>+&nbsp; /* @todo Move box-sizing into base.css under a universal selector (*) */

Why is this still a @todo?

(I asked that in ##7, on Sep 25 already!)

Sorry I must of missed that. A global box-sizing rule is a very difficult thing to retro fit onto an existing 'framework'. It has the potential to break every CSS width set in the theme or a module. IF we do decide to do this, it should be well considered in a separate issue.

+++ b/core/themes/seven/css/components/buttons.theme.css<br>@@ -0,0 +1,179 @@<br>+ * @todo replace with link--danger.<br>+ * See https://drupal.org/node/1986074#comment-7895913.

Why can't we do this here yet?

I've been trying to reduce the temptation to widen the scope of these styleguide issues beyond styling. It's not their role to perfect the markup in Drupal, that's the role of dreammarkup. We've made a few easy and subtle mark up changes here but removing the button class completely is a lot more complex and not essential for implementation. I've created a followup dreammarkup issue and referenced it in the comment #2123731: edit CSS file using .button:not(a.button--danger) so that no reset is needed on button--danger.

Status:Needs review» Needs work
Issue tags:-CSS, -Usability, -needs accessibility review, -styleguide, -styleguide-independent

The last submitted patch, seven_buttons_1986074_98.patch, failed testing.

LewisNyman’s picture

Status:Needs work» Needs review
Issue tags:+CSS, +Usability, +needs accessibility review, +styleguide, +styleguide-independent

#98: seven_buttons_1986074_98.patch queued for re-testing.

Wim Leers’s picture

Status:Needs review» Needs work

#98: Can you create follow-up issues for that first @todo that you can't fix today, so that you can link to it from the code? Then at least people won't go "WTF?" because they'll be able to see the reasoning you just posted there :) Just like you've done for the second @todo!

BarisW's "Delete a node: no red button / link?" remark of #83 is also not yet solved.

Just three more comments for the interdiff:

  1. +++ b/core/themes/seven/seven.theme
    @@ -193,7 +193,7 @@ function seven_menu_local_action($variables) {
    +    $output .=  Drupal::l($link['title'], $link['href'], $link['localized_options']);

    Two spaces instead of one, and should be \Drupal, not Drupal.

  2. +++ b/core/themes/seven/css/components/buttons.css
    @@ -13,10 +13,8 @@
      *
    - * N.B. Assumes box-sizing: border-box applied globally.

    Didn't have to be removed, but more importantly: now leaves a stray empty line above it.

  3. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -23,7 +23,7 @@
    -  font-size: 1.076923077em;
    +  font-size: 1.076923077rem;

    How is this any better? That's a crazy, crazy value. Plus, it should probably be documented why we have two successive font-size properties (probably due to lacking browser support).

Re-reviewed the entire patch, found one more extremely nitpicky problem:

+++ b/core/themes/seven/css/components/buttons.theme.css
@@ -0,0 +1,187 @@
+ * We've temporarily added the danger button here, bit of a harsh reset
+ * but we need it.

Should respect 80 col wrapping.


Did manual testing, looks great, I will RTBC as soon as the above things are fixed :)

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new17.41 KB
FAILED: [[SimpleTest]]: [MySQL] 59,398 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
new2.39 KB

Bojhan covered BarisW's "Delete a node: no red button / link?" remark in #84. It's a separate issue.

Everything else is covered though! Crossing fingers.

Wim Leers’s picture

Status:Needs review» Needs work

Point 1 is feedback/concern about the process. Point 2 is the very last nitpick…

  1. +++ b/core/themes/seven/css/components/buttons.theme.css
    @@ -23,12 +25,12 @@
    -  font-size: 1.076923077rem;
    +  font-size: 0.875rem;  /* 2 */

    Wow.
    1) That's a much less crazy value :)
    2) The result is that button text is now far less "OMG HUGE" than in the previous patch, which I must say was unexpected.
    3) The fact that such a big difference requires a non-designer like me to make obscure remarks about crazy-looking values is kind of scary to me. How is it possible that neither you nor Bojhan noticed this? A simple simplytest.me uncovers it right away!

  2. +++ b/core/themes/seven/seven.theme
    @@ -193,7 +193,7 @@ function seven_menu_local_action($variables) {
         $output .= Drupal::l($link['title'], $link['route_name'], $link['route_parameters'], $link['localized_options']);

    You forgot to update this one.

Bojhan’s picture

Yhea, we need to go form by form and see if the right styling is applied. There are many delete links that are wrong, primary buttons that are not styled primary, double primary buttons, etc. However that is not the goal of this patch, therefor I think its better to deal with them in a separate issue.

3) I thought those crazy values had a reason, I am no CSS expert :) So I ignored it.

Bojhan’s picture

doh, crosspost

LewisNyman’s picture

Status:Needs work» Needs review
StatusFileSize
new17.41 KB
FAILED: [[SimpleTest]]: [MySQL] 60,174 pass(es), 14 fail(s), and 0 exception(s).
[ View ]
new740 bytes

It's common to have numbers with many decimal places when dealing with relative values. That's the result you get when dividing 14 by 13. That changed dramatically in the last patch when we switched to rems because the root font size is much bigger than our base font size. I guess I was in a rush and missed the massive buttons.

Status:Needs review» Needs work

The last submitted patch, seven_buttons_1986074_106.patch, failed testing.

LewisNyman’s picture

When I look in menu.inc at the original theme_menu_local_action I see this:

  // @todo Remove this check and the call to l() when all pages are converted to
  //   routes.
  // @todo Figure out how to support local actions without a href properly.
  if ($link['href'] === '' && !empty($link['route_name'])) {
    $output .= Drupal::l($link['title'], $link['route_name'], $link['route_parameters'], $link['localized_options']);
  }
  else {
    $output .= l($link['title'], $link['href'], $link['localized_options']);
  }
  $output .= "</li>";

I don't really know what's going on here but I would think the lines in seven.theme should match?

LewisNyman’s picture

Issue summary:View changes

Added test pages

LewisNyman’s picture

Issue summary:View changes
StatusFileSize
new17.4 KB
PASSED: [[SimpleTest]]: [MySQL] 60,621 pass(es).
[ View ]
LewisNyman’s picture

Status:Needs work» Needs review

I've changed the theme function l() calls back to match the original theme function in menu.inc. If they don't fail there I don't see a reason to change them in the theme.

Wim Leers’s picture

Status:Needs review» Reviewed & tested by the community

Oh, it's equally inconsistent in menu.inc… then there's definitely no reason for me to insist to do things correctly. That can be a different quickfix nitpick patch.

Patch still works as expected. RTBC again!

Awesome work — thank you! :)

Bojhan’s picture

In terms of crediting designers, please make sure you credit "r5yn, Bojhan, yoroy". We spend many weeks, iterating on these buttons - fine tuning all the different styles, a11y optimisation etc.

Xano’s picture

109: seven_buttons_1986074_109.patch queued for re-testing.

webchick’s picture

Status:Reviewed & tested by the community» Fixed
StatusFileSize
new12.44 KB

Wow, this is a great patch! That new button style really "pops!" :D (Seriously; awesome work!)

The only thing that looks wonky is the node add/edit form, because it has a dropbutton next to a regular button and right now that looks like this:

Square button next to round button.

This will get cleaned up once #1989470: Dropbutton style update for Seven is in core, so I'll head over there and tag that one as an alpha target to hopefully get some more visibility on this.

In clicking through, I also found a couple of other wonky things (lots of admin pages without a primary button declared, esp. single-button forms which is weird, admin/people has not one, not two, but THREE apply buttons, admin/structure/types/add has two primary buttons, etc.) but they're not strictly related to this patch, so I'll file follow-ups for them.

Committed and pushed to 8.x. ROCK!

Dave Reid’s picture

Is it worth asking what happens when the primary button is also disabled? Does it revert to the non-primary styling? Does it not change from the primary style?

Bojhan’s picture

@Dave Reid In short, yes. https://groups.drupal.org/node/283223#Buttons is a more accurate visualisation.

Status:Fixed» Closed (fixed)

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