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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move "Undo Discard" button behind context menu #1702

Merged
merged 21 commits into from Oct 5, 2018

Conversation

@kuychaco
Member

kuychaco commented Sep 25, 2018

Description of the Change

This PR removes the Undo Discard button and hides this functionality behind a context menu that is accessed from the Unstaged Changes list header.

git_ ___src_test-repo

fullscreen_9_24_18__11_43_pm

Note: A similar context menu currently exists in the Merge Conflicts list header for bulk resolve actions.

When multiple files are selected, the context menu item is pluralized to Discard Changes in Selected Files. I chose this more verbose description (rather than simply Discard Changes) to make it clear that this would discard changes for the Unstaged Changes file list, and not for the file diff view which has separate discard state.

The Stage/Unstage all buttons no longer disappear when there are no changes. Now they simply become disabled. This gives something for the new kebab menu to sit up against.

Alternate Designs

Initially, I moved the Undo Discard button into the header and hid the text when the width was too narrow (see b82e732):

git_ ___src_test-repo

git_ ___src_test-repo

This solution still leaves room for the user to accidentally click the Undo Discard button. It's right next to the Stage All button and, depending on styling and theme, the boundary between the two buttons may not be super visible, making it easier for users to mistakenly hit the Undo discard button when trying to hit the Stage All button.

Hiding the undo-discard functionality behind a context menu on the header makes it harder to accidentally trigger this action. See images in section above.

Benefits

This UX makes it harder to accidentally undo the last discard.

Possible Drawbacks

The undo discard functionality might be less discoverable since it is hidden behind a menu. Users may be confused by the disappearance of a button from the UI.

Applicable Issues

Issue -- #1242
Supersedes PR #1252
Fixes #1260

Metrics

  • Instrument undoLastDiscard method
  • Instrument discard methods
  • Instrument UI entry points (buttons, commands, header menu)

Tests

Manual testing steps:

  • click on ... menu
  • verify that undo last discard is greyed out when there is no discard history
  • verify that "Discard Changes in Selected File" is pluralized correctly
  • verify functionality of all menu items (discard all, discard selected files, undo last discard)
  • verify that Stage/Unstage All buttons remain in UI and are disabled when there are no changes
  • verify that header context menus no longer show "Stage All" and "Discard All"

No context menu unit tests added as there is currently no way to write UI tests for context menus in electron --
electron/spectron#21

Added metrics tests for recording the following:

  • discarding all changes from the StagingView
  • discarding selected changes from the StagingView
  • undoing the last discard from the StagingView
  • discarding selected lines from the FilePatchController
  • undoing the last discard from the FilePatchController

Documentation

User Experience Research (Optional)

Some light UXR before shipping would be helpful. Especially to assess discoverability.
After shipping, we can monitor usage of the discard/undo discard functionality. It would be interesting to tweet about it and see if that affects feature usage.

Questions:

  • How's the wording of the context menu items?
  • When there are no unstaged changes the Stage All button disappears and the ellipses icon moves to the right. This might look jarring. Maybe we should keep it fixed in the middle of the header, or place it to the right of the Stage All button. If we place it to the right then the Stage All button won't line up with the Unstage All button in the section below...
  • Would vertical ellipses look better than horizontal ellipses? Note that the merge conflict section uses the horizontal ellipses for its bulk resolve menu. Also, seems like the vertical ellipses icon is not available in the octicon set bundled with Atom.

/cc @simurai for 馃挱s on 鈽濓笍

TODO:

  • Conditionally show Undo Last Discard only if there is undo history
  • Show items in menus at all times, but make unclickable if no undo history
  • Remove context menu items for header and put in kebab

Future steps:

  • Consider prompting users to confirm undo discard if head has moved since last discard
  • Update flight manual when changes hit stable
@coveralls

This comment has been minimized.

coveralls commented Sep 25, 2018

Coverage Status

Coverage decreased (-0.08%) to 81.918% when pulling f759c78 on ku-move-undo-discard-btn into 1b4c899 on master.

@kuychaco kuychaco added this to In Progress 馃敡 in Feature Sprint : 27 August - 14 September 2018 : v0.20.0 via automation Sep 25, 2018

@simurai

This comment has been minimized.

Member

simurai commented Sep 26, 2018

How's the wording of the context menu items?

The more verbose Discard Changes in Selected File feels very helpful. 馃憤 Otherwise people might think it's "Discard All".

When there are no unstaged changes the Stage All button disappears and the ellipses icon moves to the right. This might look jarring.

The Stage All and Unstage All buttons could stay visible all the time, but just disabled when nothing is staged/unstaged. Then there would be no jumping.

Would vertical ellipses look better than horizontal ellipses?

An alternative could be the kebab icon icon-three-bars, hinting at a menu.

image

But I think the ellipsis is still better because the icon on the left also has three bars (with bullets).


Something that might be a bit confusing is that:

  • the "Discard All" option is under the right-click menu
  • the "Discard Changes in Selected File" and "Undo Last Discard" option is under the ... icon

Should all 3 options be in both menus so that users don't have to think where to find what? The right-click menu would be:

Stage All Changes
---
Discard All Changes
Discard Changes in Selected File
Undo Last Discard
---
Split Up
etc.

The ellipsis icon menu would be:

Discard All Changes
Discard Changes in Selected File
Undo Last Discard
  • right-click menu 馃憠 all options
  • ellipsis icon 馃憠 dedicated menu for discarding, since "Stage All Changes" already has its dedicated button.
@simurai

This comment has been minimized.

Member

simurai commented Sep 26, 2018

Show items in menus at all times, but make unclickable if no undo history

This also sounds like a good idea. "Don't worry about discarding, you can still undo it".

simurai and others added some commits Sep 26, 2018

Use <button> element for showActionsMenu icon
Makes it look more consistent with the Stage All button
Make Stage/Unstage All buttons always visible. Disable when no changes
Co-Authored-By: David Wilson <daviwil@users.noreply.github.com>
Add "Discard All Changes" to kebab menu
Co-Authored-By: David Wilson <daviwil@users.noreply.github.com>
@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 3, 2018

ellipsis icon 馃憠 dedicated menu for discarding, since "Stage All Changes" already has its dedicated button.

@simurai agreed 馃憤

I ended up removing the "Stage All" and "Discard All" from the header context menu. Now that we have a dedicated UI element for exposing these actions, it seems unnecessary to have a right-click context menu for the same actions on the header.

@smashwilson smashwilson added this to In Progress 馃敡 in Feature Sprint : 1 October - 19 November 2018 : v0.21.0 via automation Oct 4, 2018

@smashwilson

Yesss

:shipit: :shipit: :shipit:

addEvent('undo-last-discard', {
package: 'github',
component: 'FilePatchController',
});

This comment has been minimized.

@smashwilson

smashwilson Oct 4, 2018

Member

Yess metrics

@@ -377,20 +381,30 @@ export default class StagingView extends React.Component {
this.subs.dispose();
}
getSelectedItemFilePaths() {

This comment has been minimized.

@smashwilson

smashwilson Oct 4, 2018

Member

馃憤 for refactoring this out.

const menu = new Menu();
const selectedItemCount = this.getSelectedItemFilePaths().length;

This comment has been minimized.

@smashwilson

smashwilson Oct 4, 2018

Member

This could also be this.state.selection.getSelectedItems().size to save an Array construction, yeah?

(Micro-optimization, not important 馃槅 )

This comment has been minimized.

@kuychaco

kuychaco Oct 4, 2018

Member

Indeed. I'm all for optimization 馃憤

it('records an event', function() {
const wrapper = mount(component);
sinon.stub(reporterProxy, 'addEvent');
wrapper.instance().discardLines(['once upon a time', 'there was an octocat named mona lisa']);

This comment has been minimized.

@smashwilson

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from In Progress 馃敡 to QA Review 馃敩 Oct 4, 2018

Micro-optimization getting selected item count
Co-Authored-By: Ash Wilson <smashwilson@gmail.com>
Show resolved Hide resolved lib/views/staging-view.js
const menu = new Menu();
const selectedItemCount = this.state.selection.getSelectedItems().size;
const pluralization = selectedItemCount > 1 ? 's' : '';

This comment has been minimized.

@annthurium

annthurium Oct 4, 2018

Contributor

if we ever i18n our package, all our hard coded pluralization is going to be fun to convert....

This comment has been minimized.

@kuychaco

kuychaco Oct 5, 2018

Member

Yuuuup 馃槙

@@ -797,6 +846,11 @@ export default class StagingView extends React.Component {
return;
}
addEvent('undo-last-discard', {

This comment has been minimized.

@annthurium

annthurium Oct 4, 2018

Contributor

same comment as above - would recommend refactoring this so that you're logging menu views as a ui interaction.

Add eventSource metadata to `undo-last-discard` event reporting
Co-Authored-By: Tilde Ann Thurium <annthurium@github.com>
@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 5, 2018

@annthurium what do you think of my addition of metadata for event collection in 32b0ada. I still have to do something similar for discarding changes.

@annthurium

ah, I like eventSource! That does solve the problem elegantly. Thanks for addressing my review feedback!

We should figure out a way of instrumenting context menu actions. I'll file an issue for that.

@kuychaco

This comment has been minimized.

Member

kuychaco commented Oct 5, 2018

ah, I like eventSource! That does solve the problem elegantly. Thanks for addressing my review feedback!

@annthurium Totally! Thanks for pushing me to think past my initial block on this :)

@kuychaco kuychaco merged commit 9a53fef into master Oct 5, 2018

6 checks passed

ci/circleci: beta Your tests passed on CircleCI!
Details
ci/circleci: dev Your tests passed on CircleCI!
Details
ci/circleci: stable Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.08%) to 81.918%
Details

Feature Sprint : 1 October - 19 November 2018 : v0.21.0 automation moved this from QA Review 馃敩 to Merged 鈽戯笍 Oct 5, 2018

@kuychaco kuychaco deleted the ku-move-undo-discard-btn branch Oct 5, 2018

@annthurium annthurium referenced this pull request Oct 20, 2018

Closed

v0.21.0-0 QA Review #1752

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