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

[scm] open 'diff-editors' with a single-click #6481

Merged
merged 1 commit into from
Nov 11, 2019
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Nov 1, 2019

What it does

Fixes #6479

  • updated the opening of diff-editors through the scm widget to respect the preference workbench.list.openMode. The following preference describes two possible values singleClick and doubleClick. When singleClick is enabled, clicking the change in the scm widget opens the diff-editor directly while doubleClick preference requires two clicks.
  • renamed the preference list.openMode to workbench.list.openMode.

How to test

  • start the application using a workspace under Git version control
  • verify the behavior of opening diff-editor changes with the preference:
    • "workbench.list.openMode": "singleClick" - diff-editor is opened with one click
    • "workbench.list.openMode": "doubleClick" - diff-editor is opened with two clicks

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added ui/ux issues related to user interface / user experience scm issues related to the source control manager labels Nov 1, 2019
@akosyakov
Copy link
Member

It looks like:

  • single click should open a first change
  • double click just opens

I'm not sure, we need to get a version before the scm extension and see how the git widget used to work, then fix that it works like a before. I think it got broken during this refactoring.

@vince-fugnitto
Copy link
Member Author

@akosyakov I've verified by building a v0.6.0 Theia application (pre-scm) and the single-click still does not add much value. When single-clicking, the node is selected only, while double-clicking the node is opened as a diff-editor. I'm wondering what the added value is of having the pre-existing behavior for the single-click or if we should go towards VS Code and open the diff-editor with a single-click like this PR does.

@akosyakov
Copy link
Member

@vince-fugnitto agree that we should go VS Code.

I've verified by building a v0.6.0 Theia application (pre-scm) and the single-click still does not add much value.

From code it looked that we used to open a diff-editor and select the first change. Maybe it was broken there as well. Anyway let's do as in VS Code.

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto agree that we should go VS Code.

Okay sounds great, please let me know if anything needs to be adjusted in the pull request.

@akosyakov
Copy link
Member

akosyakov commented Nov 6, 2019

I've just tested with VS Code and it actually based on

It also seems that this preference was renamed to workbench.list.openMode in VS Code. We should align it.

@vince-fugnitto
Copy link
Member Author

I've just tested with VS Code and it actually based on

It also seems that this preference was renamed to workbench.list.openMode in VS Code. We should align it.

Ahh I see. I might be mistaken but I thought perhaps that preference was more useful for them since they now support the scm widget as a tree while we do not at the moment.

@akosyakov
Copy link
Member

akosyakov commented Nov 6, 2019

Ahh I see. I might be mistaken but I thought perhaps that preference was more useful for them since they now support the scm widget as a tree while we do not at the moment.

How it is implemented does not matter from user perspective, does it? btw it was always a tree in VS Code.

@vince-fugnitto
Copy link
Member Author

Ahh I see. I might be mistaken but I thought perhaps that preference was more useful for them since they now support the scm widget as a tree while we do not at the moment.

How it is implemented does not matter from user perspective, does it? btw it was always a tree in VS Code.

Yes exactly it shouldn't matter from a user's perspective.
I see, we should most likely align as well with having a tree as well, or the option to have a tree.

@akosyakov
Copy link
Member

I see, we should most likely align as well with having a tree as well, or the option to have a tree.

I meant for now:

  • renaming list.openMode preference to workbench.list.openMode
  • use it onClick and onDoubleClick handlers to decide whether they should open

@vince-fugnitto
Copy link
Member Author

I see, we should most likely align as well with having a tree as well, or the option to have a tree.

I meant for now:

  • renaming list.openMode preference to workbench.list.openMode
  • use it onClick and onDoubleClick handlers to decide whether they should open

Sounds good, I was just wondering if it is possible to inject the corePreferences into a React component which is not injected itself. I assume I will need to pass along the value as a prop.

@vince-fugnitto
Copy link
Member Author

I see, we should most likely align as well with having a tree as well, or the option to have a tree.

I meant for now:

  • renaming list.openMode preference to workbench.list.openMode
  • use it onClick and onDoubleClick handlers to decide whether they should open

I've updated the PR based on your suggestion :)

@vince-fugnitto vince-fugnitto force-pushed the vf/GH-6479 branch 2 times, most recently from f7b11c7 to ae4b83d Compare November 7, 2019 16:50
@vince-fugnitto vince-fugnitto force-pushed the vf/GH-6479 branch 2 times, most recently from 37fac13 to f9a9e16 Compare November 8, 2019 14:19
@akosyakov
Copy link
Member

@vince-fugnitto Please test that when you click (regardless in which mode), a resource is selected.

Fixes #6479

- updated the opening of `diff-editors` from the `scm`
widget to be triggered based on the preference `workbench.list.openMode`.
- updated the preference name `list.openMode` to `workbench.list.openMode`.

Signed-off-by: vince-fugnitto <[email protected]>
@vince-fugnitto
Copy link
Member Author

@akosyakov please let me know if everything has been addressed whenever you get a chance :)

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

works nicely now! thank you

@vince-fugnitto
Copy link
Member Author

works nicely now! thank you

Thank you for your help in making it better :)

@vince-fugnitto vince-fugnitto merged commit a9c6ed4 into master Nov 11, 2019
@vince-fugnitto vince-fugnitto deleted the vf/GH-6479 branch November 11, 2019 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scm issues related to the source control manager ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[scm] single-click nodes in the scm widget should open diff-editor
2 participants