Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diff color tweaks #1949

Merged
merged 4 commits into from Feb 18, 2019

Conversation

Projects
None yet
1 participant
@simurai
Copy link
Member

simurai commented Feb 7, 2019

Description of the Change

This makes the blue-ish background color weaker when selecting the whole hunk. But keeps it stronger when selecting individual lines/words/characters.

Some additional changes:

  • Tweaked the diff colors
  • Replaced the gutter icons to be text based. The icons felt a bit too bold.

Screenshot/Gif

Before After
screen shot 2019-02-07 at 5 40 10 pm screen shot 2019-02-07 at 5 39 34 pm

diff

Alternate Designs

  1. When clicking the "See All Staged Changes" button with the mouse, we could keep the focus on the button and not move it to the first hunk. That would also match the behaviour of single file diffs.
  2. We could differentiate between making a selections of single characters/words and selecting the whole line. Maybe have two classes github-FilePatchView-line--selected and github-FilePatchView-line--partiallySelected.
  3. Separate "selecting text/code" and "select to staging". Same behaviour as in Desktop. Maybe necessary for editable diffs.
  4. Also add diff coloring (red/green) to the gutters. Then when selecting characters/words, it's still easy to see if a line is deleted or added.

/cc @kuychaco ☝️

Benefits

Easier to read the diff when the first hunk gets automatically selected in commit previews.

Possible Drawbacks

The difference between a selected hunk and selected lines might be confusing?

Applicable Issues

Closes #1933

Metrics

N/A

Tests

Tested with Atom's bundled themes and a few community themes. Certain themes work better than others.

Documentation

N/A

Release Notes

N/A

User Experience Research (Optional)

N/A

@simurai simurai requested a review from atom/github-package Feb 7, 2019

@todo

This comment was marked as resolved.

Copy link

todo bot commented Feb 7, 2019

replace icon

content: @no-newline; // TODO replace icon
}
}
}


This comment was generated by todo based on a TODO comment in d6d0000 in #1949. cc @atom.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 7, 2019

Codecov Report

Merging #1949 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1949      +/-   ##
==========================================
+ Coverage   92.12%   92.13%   +<.01%     
==========================================
  Files         188      188              
  Lines       10807    10807              
  Branches     1581     1581              
==========================================
+ Hits         9956     9957       +1     
+ Misses        851      850       -1
Impacted Files Coverage Δ
lib/github-package.js 68.48% <0%> (+0.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d846182...ed7e328. Read the comment docs.

@simurai

This comment has been minimized.

Copy link
Member Author

simurai commented Feb 18, 2019

Ok, let's merge this, regardless of potentially implementing one of the alternatives above ☝️.

@simurai simurai merged commit 81dbde7 into master Feb 18, 2019

2 checks passed

codecov/patch Coverage not affected when comparing d846182...ed7e328
Details
codecov/project 92.13% (+<.01%) compared to d846182
Details

@simurai simurai deleted the sm/diff-colors branch Feb 18, 2019

@vanessayuenn vanessayuenn referenced this pull request Mar 6, 2019

Closed

v0.27.0-0 QA Review #2005

7 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.