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

Commit Preview tweaks #1805

Merged
merged 4 commits into from Jan 2, 2019

Conversation

3 participants
@simurai
Copy link
Member

simurai commented Nov 21, 2018

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Here some tweaks carried over from #1767 (comment).

1. Tab title

Change the tab title to "Staged Changes".

Before After
image image

This makes it the same as the title in the staging view:

screen shot 2018-11-21 at 12 13 46 pm

2. Move the "See All Staged Changes" button

So that it's part of the "Staged Changes" box.

Before After
image screen shot 2018-11-21 at 12 34 42 pm

Note: This is currently just a visual hack. Under the hood, the button is still part of CommitView.

Alternate Designs

This is an alternative to #1767.

Benefits

Less confusion around clicking a "See All Staged Changes" button, but then opening a "Commit Preview" pane.

Possible Drawbacks

None, except maybe refactor all occurrences of CommitPreview.

Applicable Issues

Follow-up for #1767.

Metrics

N/A

Tests

N/A

Documentation

Documentation is tracked here #1791

Release Notes

N/A

User Experience Research (Optional)

I think the change from calling the button "Commit Preview" to "See All Staged Changes" comes from doing some user research. This PR takes it a bit further.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 21, 2018

Coverage Status

Coverage increased (+0.06%) to 85.572% when pulling a465a67 on sm/commit-preview-tweaks into cc13510 on master.

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Nov 21, 2018

Here some more options that aren't part of this PR yet, but could still be considered.

3. Remove the toggling

Is the "Hide All Staged Changes" really needed? Since the pane a pending pane, it will be replaced by another file. Or users can always close the pane with the X or cmd-w. If a user clicks on a single file above, we also don't have a "hide file" toggle.

4. Remove "Staged Changes for" prefix

In FilePatchViews, the header still says "Staged Changes for ...". Should we remove it here too? To keep it consistent with the "Commit Preview/Staged Changes" pane.

Before After
image image
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 2, 2019

Codecov Report

Merging #1805 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1805      +/-   ##
==========================================
- Coverage   91.15%   91.14%   -0.02%     
==========================================
  Files         196      196              
  Lines       10745    10745              
  Branches     1575     1575              
==========================================
- Hits         9795     9793       -2     
- Misses        950      952       +2
Impacted Files Coverage Δ
lib/items/commit-preview-item.js 100% <100%> (ø) ⬆️
lib/git-shell-out-strategy.js 87.52% <0%> (-0.36%) ⬇️

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 0e013b6...3f31f60. Read the comment docs.

@vanessayuenn vanessayuenn merged commit d02508e into master Jan 2, 2019

2 checks passed

codecov/patch 100% of diff hit (target 91.15%)
Details
codecov/project Absolute coverage decreased by -0.01% but relative coverage increased by +8.84% compared to 0e013b6
Details

@vanessayuenn vanessayuenn deleted the sm/commit-preview-tweaks branch Jan 2, 2019

This was referenced Jan 2, 2019

@smashwilson smashwilson moved this from In Progress 🔧 to Merged ☑️ in Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 Jan 3, 2019

@smashwilson smashwilson referenced this pull request Jan 3, 2019

Closed

v0.23-2 QA Review #1879

16 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment