From d4454f43a21865e785608d31da6ff4c767bfc2a1 Mon Sep 17 00:00:00 2001
From: Felix Becker <felix.b@outlook.com>
Date: Thu, 2 Apr 2020 12:13:02 +0200
Subject: [PATCH 1/5] Fix action item padding

---
 shared/src/actions/ActionsNavItems.tsx |  2 +-
 web/src/components/Breadcrumb.scss     |  1 +
 web/src/repo/RepoHeader.scss           | 20 ++++++++++++--------
 web/src/repo/RepoHeader.tsx            |  2 +-
 4 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/shared/src/actions/ActionsNavItems.tsx b/shared/src/actions/ActionsNavItems.tsx
index 7b5b3f92f5ff..c65288c95d74 100644
--- a/shared/src/actions/ActionsNavItems.tsx
+++ b/shared/src/actions/ActionsNavItems.tsx
@@ -88,7 +88,7 @@ export class ActionsNavItems extends React.PureComponent<ActionsNavItemsProps, A
             return null // loading
         }
 
-        const actionItems = getContributedActionItems(this.state.contributions, this.props.menu).map((item, i) => (
+        const actionItems = getContributedActionItems(this.state.contributions, this.props.menu).map(item => (
             <React.Fragment key={item.action.id}>
                 {' '}
                 <li className={this.props.listItemClass}>
diff --git a/web/src/components/Breadcrumb.scss b/web/src/components/Breadcrumb.scss
index a2f2b3708909..fbfe8c393ea3 100644
--- a/web/src/components/Breadcrumb.scss
+++ b/web/src/components/Breadcrumb.scss
@@ -1,5 +1,6 @@
 .breadcrumb {
     display: flex;
+    align-items: center;
 
     .part-directory {
         flex: 1 1 1em;
diff --git a/web/src/repo/RepoHeader.scss b/web/src/repo/RepoHeader.scss
index a77e0045a7dd..39c11fdebf08 100644
--- a/web/src/repo/RepoHeader.scss
+++ b/web/src/repo/RepoHeader.scss
@@ -7,23 +7,27 @@
     flex: none;
     padding-top: 0;
     padding-bottom: 0;
+    padding-right: 0;
+    align-items: stretch;
     border-style: solid;
     border-width: 0 0 1px;
 
     .navbar-nav {
         white-space: nowrap;
 
-        // Less padding above, below, and between actions.
-        .nav-item .nav-link {
-            padding: 0.425rem 0.175rem;
-            user-select: none;
+        .nav-item {
+            display: flex;
+            align-items: center;
+            margin-left: 0.125rem;
+
+            .nav-link {
+                user-select: none;
+            }
         }
     }
 
-    &__repo {
-        &-basename {
-            font-weight: bold;
-        }
+    &__repo-basename {
+        font-weight: bold;
     }
 
     &__alert {
diff --git a/web/src/repo/RepoHeader.tsx b/web/src/repo/RepoHeader.tsx
index b89ef0e79726..ae0843a7147a 100644
--- a/web/src/repo/RepoHeader.tsx
+++ b/web/src/repo/RepoHeader.tsx
@@ -226,7 +226,7 @@ export class RepoHeader extends React.PureComponent<Props, State> {
                     ))}
                 </ul>
                 <div className="repo-header__spacer" />
-                <ul className="navbar-nav navbar-nav__action-items">
+                <ul className="navbar-nav">
                     <WebActionsNavItems {...this.props} menu={ContributableMenu.EditorTitle} />
                 </ul>
                 <ul className="navbar-nav">

From 06c4064a7a996bd389baa587455bb3d0a142eb5a Mon Sep 17 00:00:00 2001
From: Felix Becker <felix.b@outlook.com>
Date: Thu, 2 Apr 2020 12:13:36 +0200
Subject: [PATCH 2/5] Fix style of action item that is not a link

Links need empty href to be styled properly.
Rewrite component to function component.
---
 .../__snapshots__/ActionItem.test.tsx.snap    |  33 ++++--
 shared/src/components/LinkOrButton.tsx        | 105 ++++++++++--------
 .../__snapshots__/LinkOrButton.test.tsx.snap  |   5 +-
 3 files changed, 82 insertions(+), 61 deletions(-)

diff --git a/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap b/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
index 189d9371863c..843d06712deb 100644
--- a/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
+++ b/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
@@ -3,8 +3,9 @@
 exports[`ActionItem actionItem variant 1`] = `
 <a
   aria-label="d"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -24,8 +25,9 @@ exports[`ActionItem actionItem variant 1`] = `
 exports[`ActionItem non-actionItem variant 1`] = `
 <a
   aria-label="d"
-  className="action-item "
+  className="action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -45,7 +47,8 @@ exports[`ActionItem non-actionItem variant 1`] = `
 exports[`ActionItem non-pressed actionItem 1`] = `
 <a
   aria-pressed={false}
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -75,7 +78,8 @@ exports[`ActionItem noop command 1`] = `
 exports[`ActionItem pressed toggle actionItem 1`] = `
 <a
   aria-pressed={true}
-  className="action-item action-item--variant-action-item action-item--pressed "
+  className="action-item action-item--variant-action-item action-item--pressed"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -89,7 +93,7 @@ exports[`ActionItem pressed toggle actionItem 1`] = `
 
 exports[`ActionItem render as link for "open" command 1`] = `
 <a
-  className="action-item "
+  className="action-item"
   href="https://example.com/bar"
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -103,7 +107,7 @@ exports[`ActionItem render as link for "open" command 1`] = `
 
 exports[`ActionItem render as link with icon for "open" command with different origin 1`] = `
 <a
-  className="action-item "
+  className="action-item"
   href="https://other.com/foo"
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -121,6 +125,7 @@ exports[`ActionItem run command 1`] = `
   aria-label="d"
   className="action-item action-item--variant-action-item disabled"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -140,8 +145,9 @@ exports[`ActionItem run command 1`] = `
 exports[`ActionItem run command 2`] = `
 <a
   aria-label="d"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -161,8 +167,9 @@ exports[`ActionItem run command 2`] = `
 exports[`ActionItem run command with error 1`] = `
 <a
   aria-label="d"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -182,8 +189,9 @@ exports[`ActionItem run command with error 1`] = `
 exports[`ActionItem run command with error with showInlineError 1`] = `
 <a
   aria-label="Error: x"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="Error: x"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -205,6 +213,7 @@ exports[`ActionItem run command with showLoadingSpinnerDuringExecution 1`] = `
   aria-label="d"
   className="action-item action-item--loading action-item--variant-action-item disabled"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -231,8 +240,9 @@ exports[`ActionItem run command with showLoadingSpinnerDuringExecution 1`] = `
 exports[`ActionItem run command with showLoadingSpinnerDuringExecution 2`] = `
 <a
   aria-label="d"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -252,8 +262,9 @@ exports[`ActionItem run command with showLoadingSpinnerDuringExecution 2`] = `
 exports[`ActionItem title element 1`] = `
 <a
   aria-label="d"
-  className="action-item action-item--variant-action-item "
+  className="action-item action-item--variant-action-item"
   data-tooltip="d"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
diff --git a/shared/src/components/LinkOrButton.tsx b/shared/src/components/LinkOrButton.tsx
index 50da04d19c42..e3a7e29a71d9 100644
--- a/shared/src/components/LinkOrButton.tsx
+++ b/shared/src/components/LinkOrButton.tsx
@@ -1,7 +1,12 @@
 import * as H from 'history'
-import * as React from 'react'
+import React, { useCallback } from 'react'
 import { Key } from 'ts-key-enum'
 import { Link } from './Link'
+import classNames from 'classnames'
+import { noop } from 'lodash'
+
+const isSelectKeyPress = (event: React.KeyboardEvent): boolean =>
+    event.key === Key.Enter && !event.ctrlKey && !event.shiftKey && !event.metaKey && !event.altKey
 
 interface Props {
     /** The link destination URL. */
@@ -41,58 +46,62 @@ interface Props {
  * It is keyboard accessible: unlike <Link> or <a>, pressing the enter key triggers it. Unlike
  * <button>, it shows a focus ring.
  */
-export class LinkOrButton extends React.PureComponent<Props> {
-    public render(): JSX.Element | null {
-        const className = `${this.props.className === undefined ? 'nav-link' : this.props.className} ${
-            this.props.disabled ? 'disabled' : ''
-        }`
-
-        const commonProps: React.AnchorHTMLAttributes<HTMLAnchorElement> & {
-            'data-tooltip': string | undefined
-            onAuxClick?: React.MouseEventHandler<HTMLAnchorElement>
-        } = {
-            className,
-            'data-tooltip': this.props['data-tooltip'],
-            'aria-label': this.props['data-tooltip'],
-            role: typeof this.props.pressed === 'boolean' ? 'button' : undefined,
-            'aria-pressed': this.props.pressed,
-            tabIndex: 0,
-            onClick: this.onAnchorClick,
-            onKeyPress: this.onAnchorKeyPress,
-        }
+export const LinkOrButton: React.FunctionComponent<Props> = ({
+    className = 'nav-link',
+    to,
+    target,
+    disabled,
+    pressed,
+    'data-tooltip': tooltip,
+    onSelect = noop,
+    children,
+}) => {
+    // We need to set up a keypress listener because <a onclick> doesn't get
+    // triggered by enter.
+    const onAnchorKeyPress: React.KeyboardEventHandler<HTMLAnchorElement> = useCallback(
+        event => {
+            if (isSelectKeyPress(event)) {
+                onSelect(event)
+            }
+        },
+        [onSelect]
+    )
 
-        if (!this.props.to) {
-            // Use onAuxClick so that middle-clicks are caught.
-            commonProps.onAuxClick = this.onAnchorClick
+    const commonProps: React.AnchorHTMLAttributes<HTMLAnchorElement> & {
+        'data-tooltip': string | undefined
+    } = {
+        className: classNames(className, disabled && 'disabled'),
+        'data-tooltip': tooltip,
+        'aria-label': tooltip,
+        role: typeof pressed === 'boolean' ? 'button' : undefined,
+        'aria-pressed': pressed,
+        tabIndex: 0,
+        onClick: onSelect,
+        onKeyPress: onAnchorKeyPress,
+    }
 
-            // Render using an <a> with no href, so that we get a focus ring (when using Bootstrap).
-            // We need to set up a keypress listener because <a onclick> doesn't get triggered by
-            // enter.
-            return <a {...commonProps}>{this.props.children}</a>
-        }
+    const onClickPreventDefault: React.MouseEventHandler<HTMLAnchorElement> = useCallback(
+        event => {
+            // Prevent default action of reloading page because of empty href
+            event.preventDefault()
+            onSelect(event)
+        },
+        [onSelect]
+    )
 
+    if (!to) {
         return (
-            <Link {...commonProps} to={this.props.to} target={this.props.target}>
-                {this.props.children}
-            </Link>
+            // Need empty href for styling reasons
+            // Use onAuxClick so that middle-clicks are caught.
+            <a href="" {...commonProps} onClick={onClickPreventDefault} onAuxClick={onClickPreventDefault}>
+                {children}
+            </a>
         )
     }
 
-    private onAnchorClick: React.MouseEventHandler<HTMLAnchorElement> = e => {
-        if (this.props.onSelect) {
-            this.props.onSelect(e)
-        }
-    }
-
-    private onAnchorKeyPress: React.KeyboardEventHandler<HTMLAnchorElement> = e => {
-        if (isSelectKeyPress(e)) {
-            if (this.props.onSelect) {
-                this.props.onSelect(e)
-            }
-        }
-    }
-}
-
-function isSelectKeyPress(e: React.KeyboardEvent): boolean {
-    return e.key === Key.Enter && !e.ctrlKey && !e.shiftKey && !e.metaKey && !e.altKey
+    return (
+        <Link {...commonProps} to={to} target={target}>
+            {children}
+        </Link>
+    )
 }
diff --git a/shared/src/components/__snapshots__/LinkOrButton.test.tsx.snap b/shared/src/components/__snapshots__/LinkOrButton.test.tsx.snap
index cca70ec1cded..446fb3ecfaf3 100644
--- a/shared/src/components/__snapshots__/LinkOrButton.test.tsx.snap
+++ b/shared/src/components/__snapshots__/LinkOrButton.test.tsx.snap
@@ -2,7 +2,8 @@
 
 exports[`LinkOrButton render a button when "to" is undefined 1`] = `
 <a
-  className="nav-link "
+  className="nav-link"
+  href=""
   onAuxClick={[Function]}
   onClick={[Function]}
   onKeyPress={[Function]}
@@ -14,7 +15,7 @@ exports[`LinkOrButton render a button when "to" is undefined 1`] = `
 
 exports[`LinkOrButton render a link when "to" is set 1`] = `
 <a
-  className="nav-link "
+  className="nav-link"
   href="http://example.com"
   onClick={[Function]}
   onKeyPress={[Function]}

From 11f03042bdff3f302ca8a126eb92fdb8cc9d180d Mon Sep 17 00:00:00 2001
From: Felix Becker <felix.b@outlook.com>
Date: Thu, 2 Apr 2020 15:44:34 +0200
Subject: [PATCH 3/5] Clip too wide action items

---
 shared/src/actions/ActionItem.scss | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/shared/src/actions/ActionItem.scss b/shared/src/actions/ActionItem.scss
index 7f7aed48b126..b558733289ef 100644
--- a/shared/src/actions/ActionItem.scss
+++ b/shared/src/actions/ActionItem.scss
@@ -1,5 +1,9 @@
 .action-item {
     height: 100%;
+    max-width: 10rem;
+    text-overflow: ellipsis;
+    overflow: hidden;
+
     &__content {
         display: flex;
         align-items: center;

From b6697bf211e7c030481ce9ab43882d28f2f174d1 Mon Sep 17 00:00:00 2001
From: Felix Becker <felix.b@outlook.com>
Date: Thu, 2 Apr 2020 15:56:15 +0200
Subject: [PATCH 4/5] Fix spacing between icon and label

---
 shared/src/actions/ActionItem.tsx             | 10 +++---
 .../__snapshots__/ActionItem.test.tsx.snap    | 31 ++++++++-----------
 .../ActionsNavItems.test.tsx.snap             |  1 +
 3 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/shared/src/actions/ActionItem.tsx b/shared/src/actions/ActionItem.tsx
index d2c139aa27ce..4df14721f533 100644
--- a/shared/src/actions/ActionItem.tsx
+++ b/shared/src/actions/ActionItem.tsx
@@ -152,8 +152,7 @@ export class ActionItem extends React.PureComponent<ActionItemProps, State> {
                             alt={this.props.action.actionItem.iconDescription}
                             className={this.props.iconClassName}
                         />
-                    )}
-                    {this.props.action.actionItem.iconURL && this.props.action.actionItem.label && <>&nbsp;</>}
+                    )}{' '}
                     {this.props.action.actionItem.label}
                 </>
             )
@@ -161,10 +160,9 @@ export class ActionItem extends React.PureComponent<ActionItemProps, State> {
         } else {
             content = (
                 <>
-                    {this.props.action.iconURL && <img src={this.props.action.iconURL} className="icon-inline" />}
-                    {this.props.action.iconURL && (this.props.action.category || this.props.action.title) && (
-                        <>&nbsp;</>
-                    )}
+                    {this.props.action.iconURL && (
+                        <img src={this.props.action.iconURL} className={this.props.iconClassName} />
+                    )}{' '}
                     {this.props.action.category ? `${this.props.action.category}: ` : ''}
                     {this.props.action.title}
                 </>
diff --git a/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap b/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
index 843d06712deb..56bac1573b91 100644
--- a/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
+++ b/shared/src/actions/__snapshots__/ActionItem.test.tsx.snap
@@ -12,10 +12,9 @@ exports[`ActionItem actionItem variant 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -34,10 +33,9 @@ exports[`ActionItem non-actionItem variant 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -55,6 +53,7 @@ exports[`ActionItem non-pressed actionItem 1`] = `
   role="button"
   tabIndex={0}
 >
+   
   b
    
 </a>
@@ -66,10 +65,9 @@ exports[`ActionItem noop command 1`] = `
   data-tooltip="d"
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
 </span>
@@ -86,6 +84,7 @@ exports[`ActionItem pressed toggle actionItem 1`] = `
   role="button"
   tabIndex={0}
 >
+   
   b
    
 </a>
@@ -99,6 +98,7 @@ exports[`ActionItem render as link for "open" command 1`] = `
   onKeyPress={[Function]}
   tabIndex={0}
 >
+   
   
   t
    
@@ -113,6 +113,7 @@ exports[`ActionItem render as link with icon for "open" command with different o
   onKeyPress={[Function]}
   tabIndex={0}
 >
+   
   
   t
    
@@ -132,10 +133,9 @@ exports[`ActionItem run command 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -154,10 +154,9 @@ exports[`ActionItem run command 2`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -176,10 +175,9 @@ exports[`ActionItem run command with error 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -198,10 +196,9 @@ exports[`ActionItem run command with error with showInlineError 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -220,10 +217,9 @@ exports[`ActionItem run command with showLoadingSpinnerDuringExecution 1`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
@@ -249,10 +245,9 @@ exports[`ActionItem run command with showLoadingSpinnerDuringExecution 2`] = `
   tabIndex={0}
 >
   <img
-    className="icon-inline"
     src="u"
   />
-   
+   
   g: 
   t
    
diff --git a/shared/src/actions/__snapshots__/ActionsNavItems.test.tsx.snap b/shared/src/actions/__snapshots__/ActionsNavItems.test.tsx.snap
index 4c0501ce611c..b7af37a053c9 100644
--- a/shared/src/actions/__snapshots__/ActionsNavItems.test.tsx.snap
+++ b/shared/src/actions/__snapshots__/ActionsNavItems.test.tsx.snap
@@ -8,6 +8,7 @@ Array [
       className="action-item  action-item--variant-action-item"
       data-tooltip="This is Action A"
     >
+       
       Action A
     </span>
   </li>,

From 3e7c5bd0aaab12cafade5eac314179aeec0e399f Mon Sep 17 00:00:00 2001
From: Felix Becker <felix.b@outlook.com>
Date: Thu, 2 Apr 2020 17:13:26 +0200
Subject: [PATCH 5/5] Add hover background effect

---
 web/src/repo/RepoHeader.scss | 37 +++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

diff --git a/web/src/repo/RepoHeader.scss b/web/src/repo/RepoHeader.scss
index 39c11fdebf08..b687b6092b1a 100644
--- a/web/src/repo/RepoHeader.scss
+++ b/web/src/repo/RepoHeader.scss
@@ -18,7 +18,19 @@
         .nav-item {
             display: flex;
             align-items: center;
-            margin-left: 0.125rem;
+
+            // Have a small gap between buttons so they are visually distinct when pressed
+            // stylelint-disable-next-line declaration-property-unit-whitelist
+            margin-left: 1px;
+
+            &:hover {
+                .theme-dark & {
+                    background: $color-bg-1;
+                }
+                .theme-light & {
+                    background: $color-light-bg-2;
+                }
+            }
 
             .nav-link {
                 user-select: none;
@@ -52,24 +64,23 @@
     &__spacer {
         flex: 1 1 0;
     }
-}
 
-.theme-dark {
-    .repo-header {
+    .theme-dark & {
         border-color: #233043;
     }
-    .action-item--pressed {
-        background-color: $color-bg-3;
-        color: $color-text-3;
-    }
-}
 
-.theme-light {
-    .repo-header {
+    .theme-light & {
         border-color: $color-light-border;
     }
+
     .action-item--pressed {
-        background-color: $color-light-bg-3;
-        color: $color-light-text-1;
+        .theme-dark & {
+            background-color: $color-bg-2;
+            color: $color-text-3;
+        }
+        .theme-light & {
+            background-color: $color-light-bg-4;
+            color: $color-light-text-1;
+        }
     }
 }