Merge pull request desktop#3000 from desktop/diff-contrasts

Increase diff contrast and improve gutter behavior

app/src/ui/diff/diff-line-gutter.tsx

@@ -65,6 +65,21 @@ interface IDiffGutterProps {
   readonly onMouseMove: (index: number) => void
 }
 
+interface IDiffGutterState {
+  /**
+   * Whether or not the diff line gutter should render as hovered,
+   * i.e. highlighted. This is used when moused over directly or
+   * when the hunk that this line is part of is hovered.
+   */
+  readonly hover: boolean
+
+  /**
+   * Whether or not the diff line gutter should render that it's
+   * selected, i.e. included for commit.
+   */
+  readonly selected: boolean
+}
+
 /**
  * Detect if mouse cursor is within the range
  */
@@ -83,9 +98,21 @@ function isMouseCursorNearEdge(ev: MouseEvent): boolean {
 }
 
 /** The gutter for a diff's line. */
-export class DiffLineGutter extends React.Component<IDiffGutterProps, {}> {
+export class DiffLineGutter extends React.Component<
+  IDiffGutterProps,
+  IDiffGutterState
+> {
   private elem_?: HTMLSpanElement
 
+  public constructor(props: IDiffGutterProps) {
+    super(props)
+
+    this.state = {
+      hover: false,
+      selected: this.props.isIncluded,
+    }
+  }
+
   /**
    * Compute the width for the current element
    */
@@ -100,34 +127,26 @@ export class DiffLineGutter extends React.Component<IDiffGutterProps, {}> {
    * Indicate whether the current gutter element is selected
    */
   public isIncluded(): boolean {
-    return this.props.line.isIncludeableLine() && this.props.isIncluded
+    return this.props.line.isIncludeableLine() && this.state.selected
   }
 
   /**
    * Set (or unset) the hover styling of the diff gutter
    */
-  public setHover(visible: boolean) {
+  public setHover(hover: boolean) {
     // only show the hover effect if the line isn't context
     if (!this.props.line.isIncludeableLine()) {
       return
     }
 
-    if (visible) {
-      this.setClass(hoverCssClass)
-    } else {
-      this.unsetClass(hoverCssClass)
-    }
+    this.setState({ hover })
   }
 
   /**
    * Set (or unset) the selected styling of the diff gutter
    */
-  public setSelected(visible: boolean) {
-    if (visible) {
-      this.setClass(selectedLineClass)
-    } else {
-      this.unsetClass(selectedLineClass)
-    }
+  public setSelected(selected: boolean) {
+    this.setState({ selected })
   }
 
   private getLineClassName(): string {
@@ -149,8 +168,17 @@ export class DiffLineGutter extends React.Component<IDiffGutterProps, {}> {
   private getLineClass(): string {
     const lineClass = this.getLineClassName()
     const selectedClass = this.isIncluded() ? selectedLineClass : null
-
-    return classNames('diff-line-gutter', lineClass, selectedClass)
+    const hoverClass = this.state.hover ? hoverCssClass : null
+
+    return classNames(
+      'diff-line-gutter',
+      lineClass,
+      selectedClass,
+      hoverClass,
+      {
+        'read-only': this.props.readOnly,
+      }
+    )
   }
 
   private updateHoverState(isRangeSelection: boolean, isActive: boolean) {
@@ -166,18 +194,6 @@ export class DiffLineGutter extends React.Component<IDiffGutterProps, {}> {
     }
   }
 
-  private setClass(cssClass: string) {
-    if (this.elem_) {
-      this.elem_.classList.add(cssClass)
-    }
-  }
-
-  private unsetClass(cssClass: string) {
-    if (this.elem_) {
-      this.elem_.classList.remove(cssClass)
-    }
-  }
-
   private mouseEnterHandler = (ev: MouseEvent) => {
     ev.preventDefault()
 
@@ -240,19 +256,17 @@ export class DiffLineGutter extends React.Component<IDiffGutterProps, {}> {
       return
     }
 
-    if (elem) {
+    // no point handling mouse events on context lines
+    if (elem && this.props.line.isIncludeableLine()) {
       elem.addEventListener('mouseenter', this.mouseEnterHandler)
       elem.addEventListener('mouseleave', this.mouseLeaveHandler)
       elem.addEventListener('mousemove', this.mouseMoveHandler)
-
-      // no point handling mousedown events on context lines
-      if (this.props.line.isIncludeableLine()) {
-        elem.addEventListener('mousedown', this.mouseDownHandler)
-      }
+      elem.addEventListener('mousedown', this.mouseDownHandler)
     } else {
       // this callback fires a second time when the DOM element
-      // is unmounted, so we can use this as a chance to cleanup
-
+      // is unmounted, so we can use this as a chance to cleanup.
+      // We unsubscribe without checking for isIncludeableLine since
+      // that might have changed underneath us
       if (this.elem_) {
         this.elem_.removeEventListener('mouseenter', this.mouseEnterHandler)
         this.elem_.removeEventListener('mouseleave', this.mouseLeaveHandler)

app/src/ui/diff/diff-mode.ts

@@ -13,6 +13,7 @@ const TokenNames: { [key: string]: string | null } = {
   '+': 'diff-add',
   '-': 'diff-delete',
   '@': 'diff-hunk',
+  ' ': 'diff-context',
 }
 
 /** Get the mode for diffs. */

app/src/ui/diff/index.tsx

@@ -200,9 +200,8 @@ export class Diff extends React.Component<IDiffProps, {}> {
       // onscreen
       const viewport = this.codeMirror.getScrollInfo()
       const top = viewport.top
-      const cm = this.codeMirror as any
 
-      const row = cm.lineAtHeight(top, 'local')
+      const row = this.codeMirror.lineAtHeight(top, 'local')
       const element = this.cachedGutterElements.get(row)
 
       if (!element) {

app/src/ui/diff/selection/range-selection-strategy.ts

@@ -27,7 +27,7 @@ export class RangeSelection implements ISelectionStrategy {
   }
 
   public paint(elements: Map<number, DiffLineGutter>) {
-    range(this._start, this._end).forEach(row => {
+    range(this._start, this._end + 1).forEach(row => {
       const element = elements.get(row)
 
       if (!element) {
@@ -39,9 +39,7 @@ export class RangeSelection implements ISelectionStrategy {
         return
       }
 
-      const selected = this._desiredSelection
-
-      element.setSelected(selected)
+      element.setSelected(this._desiredSelection)
     })
   }
 

app/styles/_variables.scss

@@ -220,6 +220,8 @@
    * Diff view
    */
 
+  --diff-line-padding-y: 2px;
+
   --diff-text-color: $gray-900;
   --diff-border-color: $gray-200;
   --diff-gutter-color: $gray-200;
@@ -232,28 +234,41 @@
   --diff-selected-gutter-color: $blue-600;
   --diff-selected-text-color: var(--background-color);
 
-  --diff-add-background-color: $green-000;
-  --diff-add-border-color: $green-200;
-  --diff-add-gutter-color: $green-200;
+  --diff-add-background-color: darken($green-000, 2%);
+  --diff-add-border-color: $green-300;
+  --diff-add-gutter-color: $green-300;
+  --diff-add-gutter-background-color: darken($green-100, 3%);
   --diff-add-inner-background-color: #acf2bd;
   --diff-add-text-color: var(--diff-text-color);
 
   --diff-delete-background-color: $red-000;
-  --diff-delete-border-color: $red-100;
+  --diff-delete-border-color: $red-200;
   --diff-delete-gutter-color: $red-200;
+  --diff-delete-gutter-background-color: $red-100;
   --diff-delete-inner-background-color: #fdb8c0;
   --diff-delete-text-color: var(--diff-text-color);
 
   --diff-hunk-background-color: $blue-000;
   --diff-hunk-border-color: $blue-200;
-  --diff-hunk-gutter-color: $blue-200;
+  --diff-hunk-gutter-color: darken($blue-200, 5%);
+  --diff-hunk-gutter-background-color: $blue-100;
   --diff-hunk-text-color: $gray;
 
   --diff-hover-background-color: $blue-300;
   --diff-hover-border-color: $blue-400;
   --diff-hover-gutter-color: $blue-400;
   --diff-hover-text-color: var(--background-color);
 
+  --diff-add-hover-background-color: $green-300;
+  --diff-add-hover-border-color: $green-400;
+  --diff-add-hover-gutter-color: $green-400;
+  --diff-add-hover-text-color: var(--text-color);
+
+  --diff-delete-hover-background-color: $red-200;
+  --diff-delete-hover-border-color: $red-300;
+  --diff-delete-hover-gutter-color: $red-300;
+  --diff-delete-hover-text-color: var(--text-color);
+
   // Note that this duration *must* match the `UndoCommitAnimationTimeout`
   // specified in `changes/index.tsx`.
   --undo-animation-duration: 500ms;

app/styles/ui/_diff.scss

@@ -36,7 +36,9 @@
       // Add a little bit of space to the left of code diff
       // `padding-left` here means mouse move events are not raised
       padding-left: var(--spacing-half);
-      align-self: center;
+      padding-top: var(--diff-line-padding-y);
+      padding-bottom: var(--diff-line-padding-y);
+      display: inline-block;
     }
 
     svg.no-newline {
@@ -66,24 +68,39 @@
 // The container element which holds the before and after
 // diff-line-number spans.
 .diff-line-gutter {
-  line-height: normal;
   display: flex;
   height: 100%;
 
+  &.diff-add {
+    background: var(--diff-add-gutter-background-color);
+  }
+
+  &.diff-delete {
+    background: var(--diff-delete-gutter-background-color);
+  }
+
+  &.diff-hunk {
+    background: var(--diff-hunk-gutter-background-color);
+  }
+
   .before {
     border-right: 1px solid var(--diff-border-color);
   }
 
   .after {
     border-right: 4px solid var(--diff-border-color);
   }
+
+  &.read-only .after {
+    border-right-width: 1px;
+  }
 }
 
 .diff-line-number {
   display: inline-block;
   color: var(--diff-line-number-color);
   width: var(--diff-line-number-column-width);
-  padding: var(--spacing-third) var(--spacing-half);
+  padding: var(--diff-line-padding-y) var(--spacing-half);
   text-align: right;
 }
 
@@ -190,6 +207,32 @@
       border-color: var(--diff-hover-gutter-color);
     }
   }
+
+  &:not(.diff-line-selected) {
+    &.diff-add {
+      .diff-line-number {
+        background: var(--diff-add-hover-background-color);
+        border-color: var(--diff-add-hover-border-color);
+        color: var(--diff-add-hover-text-color);
+
+        &:last-child {
+          border-color: var(--diff-add-hover-gutter-color);
+        }
+      }
+    }
+
+    &.diff-delete {
+      .diff-line-number {
+        background: var(--diff-delete-hover-background-color);
+        border-color: var(--diff-delete-hover-border-color);
+        color: var(--diff-delete-hover-text-color);
+
+        &:last-child {
+          border-color: var(--diff-delete-hover-gutter-color);
+        }
+      }
+    }
+  }
 }
 
 // Windows has custom scroll bars, see _scroll.scss