Skip to content

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.

5 participants