Buttons style update

Patch incoming; give me two days.

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.

@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?

tweakage

@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 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.

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.

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

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

@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.

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

@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?

meta issue added

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.

Re-roll

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?

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.

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

I think my only major complaint is the reduced line height on local actions ("Add link" in this picture)

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

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.

Re-roll

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.

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

Trying again with a more recent version of 8.x

#43: seven_buttons_1986074_40.patch queued for re-testing.

Rerolled

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.

@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!

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.

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

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.

Looks like there's an answer here.

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.

Whoops! I mixed up the file names.

+++ 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.

Updated issue summary.

#57: seven_buttons_1986074_56.patch queued for re-testing.

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

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

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.

Sets the danger button font-weight at 400.

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

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

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

Checked with sam on IRC, all good for now.

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.

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');
    

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

+++ 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 :)

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?

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

Incorporated comments from #77 and #79.

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

I believe this needs a little bit more love.

Some screenshots.

Performance page: good.

Save theme config: variant 1

Save theme config: variant 2

Delete a node: no red button / link?

Edit node: do we need three different styles?

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.

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

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

Looks like theme_menu_local_action shifted. Good catch!

My mistake, messed up my branches.

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
   

   .

Thanks.

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

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…?

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.

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

#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 :)

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.

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.

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.

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?

Added test pages

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.

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! :)

109: seven_buttons_1986074_109.patch queued for re-testing.

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:

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!

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?