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

Improve Merge Editor Story For Files Having/Getting Conflict Markers #153800

Closed
joaomoreno opened this issue Jun 30, 2022 · 20 comments · Fixed by #158377
Closed

Improve Merge Editor Story For Files Having/Getting Conflict Markers #153800

joaomoreno opened this issue Jun 30, 2022 · 20 comments · Fixed by #158377
Assignees
Labels
feature-request Request for new features or functionality merge-editor on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@joaomoreno
Copy link
Member

Recording 2022-06-30 at 12 44 23

@jrieken
Copy link
Member

jrieken commented Jun 30, 2022

My plan is to use the existing merge-conflict extension to add a (strong) button or other UI element to re-open the file in the merge editor.

@jrieken jrieken added this to the July 2022 milestone Jun 30, 2022
@jrieken jrieken added feature-request Request for new features or functionality merge-editor labels Jun 30, 2022
@hediet hediet changed the title Merge editor: If an open file becomes conflicted, should that editor input be replaced with the merge editor input? Improve Merge Editor Story For Files Having/Getting Conflict Markers Jul 18, 2022
@lszomoru lszomoru modified the milestones: July 2022, August 2022 Jul 25, 2022
@hediet
Copy link
Member

hediet commented Aug 9, 2022

@lszomoru
Copy link
Member

@hediet, I believe that this can also be closed as a duplicate, no?

@hediet
Copy link
Member

hediet commented Aug 15, 2022

This is not a duplicate, as this is about opening the 3 way merge editor from the decorations.

@jrieken
Copy link
Member

jrieken commented Aug 17, 2022

This is how it looks with #158377 (draft)

Screen.Recording.2022-08-17.at.16.23.24.mov

@misolori @daviddossett This is a bit UI questionable - I am using a command with a label only (no icon) so that it stands out more... Maybe we find a prominent (colored?) icon instead or maybe we think about prominent actions in general (similar to the default command effort, tho this is more complex)

@miguelsolorio
Copy link
Contributor

Didn't we previously have an option in the codelens to open the merge editor or did that not get added?

@jrieken
Copy link
Member

jrieken commented Aug 17, 2022

No, that didn't get added. We have the "normal/old" lenses to resolve individual conflicts but @hediet and I weren't sold on the idea of repeating a "open in merge editor"-command there. Code lens should be contextual and not global commands.

@daviddossett daviddossett self-assigned this Aug 17, 2022
@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label Aug 18, 2022
@miguelsolorio
Copy link
Contributor

Tried this out today and it definitely feels strange with just the label, I think an icon might help it go a long way:

CleanShot 2022-08-18 at 10 25 20

@daviddossett
Copy link
Contributor

👍 for using an icon to at least be consistent with other icon buttons w/ labels (e.g. notebooks toolbar). Originally we had this icon along to toggle between the raw file and merge editor views. Could use it with this new label too. git-merge felt out of place as an icon choice to my eyes.

CleanShot 2022-08-18 at 10 42 57@2x

@vscodenpa vscodenpa removed the unreleased Patch has not yet been released in VS Code Insiders label Aug 19, 2022
@vscodenpa vscodenpa added the insiders-released Patch has been released in VS Code Insiders label Aug 19, 2022
@jrieken
Copy link
Member

jrieken commented Aug 19, 2022

Text and icon is surprisingly hard because we always default to the icon (when there is one). This coming from an extension makes life event harder. The challenge (and reason for using text) is that we want something that grabs attention. @daviddossett @misolori how about a prominent, colored icon? Or maybe the notion of a default/prominent button, or ultimately use the "blue" in-editor button? Like so

Screenshot 2022-08-19 at 16 03 13

@daviddossett
Copy link
Contributor

or ultimately use the "blue" in-editor button

This was my next idea too. I like this since even with text I'm not sure I'd always notice the button in the toolbar.

I do think it would be interesting to explore the notion of a single prominent action in the editor toolbar for use beyond just this scenario. Another example: I think the Python extension uses a green "run" icon but I recall that being hard for users to spot. I'll create an issue to explore this as a general pattern.

@jrieken jrieken reopened this Aug 22, 2022
@jrieken
Copy link
Member

jrieken commented Aug 22, 2022

Re-opening and fixing this with the "blue button"

@vscodenpa vscodenpa removed the insiders-released Patch has been released in VS Code Insiders label Aug 22, 2022
@jrieken
Copy link
Member

jrieken commented Aug 22, 2022

The blue button feels more natural. This is what I'll be PR'ing

Screen.Recording.2022-08-22.at.12.14.30.mov

@gjsjohnmurray
Copy link
Contributor

Looks good but is there a risk that if I accidentally click twice, slowly, over "Open in Merge Editor" I'll inadvertently trigger "Accept Merge" on the merge editor?

jrieken added a commit that referenced this issue Aug 22, 2022
* rename proposed `MergeToolbar` to `EditorContent` menu
* adopt GIT and sync conflicts usage
* use content menu for "open in 3wm" command
* add new `ctxIsMergeResultEditor` context key

Better fix for #153800
jrieken added a commit that referenced this issue Aug 22, 2022
* Expose editor "blue button" as menu

* rename proposed `MergeToolbar` to `EditorContent` menu
* adopt GIT and sync conflicts usage
* use content menu for "open in 3wm" command
* add new `ctxIsMergeResultEditor` context key

Better fix for #153800

* fix typo
@jrieken jrieken closed this as completed Aug 22, 2022
@jrieken
Copy link
Member

jrieken commented Aug 22, 2022

Looks good but is there a risk that if I accidentally click twice,

🤷 don't click twice? I see the risk but it's also not unique to this UX but possible with any button that causes an in-place change

@IllusionMH
Copy link
Contributor

@jrieken is there a chance to hide/disable Accept Merge button for couple of seconds after switch?

@jrieken jrieken added the verification-needed Verification of issue is requested label Aug 22, 2022
@jrieken
Copy link
Member

jrieken commented Aug 22, 2022

To verify (see #153800 (comment))

  • open a file
  • cause a merge conflict in that file
  • make sure the blue button with "Open in Merge Editor" shows

@DonJayamanne DonJayamanne added the verified Verification succeeded label Aug 24, 2022
@hediet
Copy link
Member

hediet commented Aug 25, 2022

🤷 don't click twice?

Actually it happens to me from time to time that I accidentally double-click, especially after the gym and with a sensitive mouse ;)

@daviddossett
Copy link
Contributor

After using this for a few days I think I actually preferred the toolbar option. Opened #159195 to explore this further as a general pattern.

@jrieken jrieken added the on-release-notes Issue/pull request mentioned in release notes label Aug 26, 2022
@vai0
Copy link

vai0 commented Sep 12, 2022

also finding it a bit annoying there's a big blue "Open in Merge Editor" button when I already know about it, and prefer not to use it.

@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality merge-editor on-release-notes Issue/pull request mentioned in release notes verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.