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

avoid resolving conflicts using "Open in External Editor" flow if no editors found #6109

Merged
merged 9 commits into from
Nov 8, 2018

Conversation

shiftkey
Copy link
Member

@shiftkey shiftkey commented Nov 6, 2018

Overview

Closes #6060

Description

  • lift error out of findEditorOrDefault, make App responsible for that, and return null if no editors can be found
  • update MergeConflictsDialog to resolve the editor after mounting, and not show the button if the editor cannot be found
  • test happy path when an editor can be found -> user can resolve conflicts in editor
  • test sad path when not editors can be found -> buttons are disabled

Release notes

This is related to a new feature that hasn't yet made it to production

Notes: no-notes

@shiftkey shiftkey added this to the 1.5.0 milestone Nov 6, 2018
@shiftkey
Copy link
Member Author

shiftkey commented Nov 6, 2018

Here's what the conflicts dialog looks like when we're unable to find any editor:

@shiftkey
Copy link
Member Author

shiftkey commented Nov 6, 2018

@billygriffin @ampinsk @outofambit just going to call out what I'd like input on from a UX side as part of addressing this bug:

  • given all the whitespace on the right-hand column when you don't have an active editor, should we try and make use of that space? i'll see if i can put together a screenshot of what this looks like with long paths
  • should we make the command line link more prominent - i.e. above the list of files - if that's what they need to use?

@billygriffin
Copy link
Contributor

Recap of discussion with @shiftkey, @ampinsk, @tierninho, and me:

From @shiftkey's screenshot in #6109 (comment), we'll add in buttons as normal next to each conflict.

  • The buttons will say "Open in editor," cased appropriately
  • The buttons will be disabled
  • Hovering over each button will have a tooltip that says: "No editor configured in Preferences > Advanced"

@shiftkey
Copy link
Member Author

shiftkey commented Nov 6, 2018

One extra thing:

"No editor configured in Preferences > Advanced"

This is a platform-specific label too (Preferences on macOS, Options on the others)

@shiftkey shiftkey force-pushed the dont-show-editor-if-no-editor-found branch from e0faa1c to 4fb6017 Compare November 6, 2018 19:42
@shiftkey
Copy link
Member Author

shiftkey commented Nov 6, 2018

Some updated screenshots based on the feedback from above:

@shiftkey shiftkey added the ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 6, 2018
@tierninho
Copy link
Contributor

LGTM!

@outofambit outofambit self-assigned this Nov 7, 2018
Copy link
Contributor

@outofambit outofambit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants