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

Debug: Added collapse all in debug console Fixes: #38109 #43987

Merged
merged 6 commits into from Feb 22, 2018

Conversation

Projects
None yet
2 participants
@svitlana-galianova
Contributor

svitlana-galianova commented Feb 19, 2018

Added a "Collapse All" button in DEBUG CONSOLE in both light and dark color schemes.
Let me know if there is any way I can improve this code, issue #38109

collapsealllight

collapseall

@isidorn

This comment has been minimized.

Contributor

isidorn commented Feb 20, 2018

@svitlana-galianova thanks a lot for your PR. Seems to work nicely, however in the issue I already mentioned that I would only accept if this is added to the context menu, not to the title area because I do not want to introduce clutter in the debug console title area (limited space when panel is vertical).

So in order for this PR to be merged, I would require two things:

  • Action added in the context menu, not the title area
  • Remove all your formating changes to unrelated files, in this PR you can see all the changes you have done. A nice PR always touches the minimal amount of code needed for the change, that is not the case here as a lot of lines have been formated. All that needs to be reverted in order for us to accept this

After this has been done I can provide more detailed feedback if needed.
Thanks 🍹

svitlana-galianova added some commits Feb 20, 2018

@svitlana-galianova

This comment has been minimized.

Contributor

svitlana-galianova commented Feb 20, 2018

I have fixed an indentation. But I don't know what context menu is, could you please provide a screenshot or code pointer to that? Thank you!

@isidorn

This comment has been minimized.

Contributor

isidorn commented Feb 20, 2018

@svitlana-galianova cool the identation is now fixed. The context menu is when you right click anywhere in the debug repl
Here's a code pointer https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/parts/debug/electron-browser/replViewer.ts#L436

screen shot 2018-02-20 at 16 38 59

@svitlana-galianova

This comment has been minimized.

Contributor

svitlana-galianova commented Feb 20, 2018

Here is a gif how it works now!

collapseall

@isidorn

This comment has been minimized.

Contributor

isidorn commented Feb 20, 2018

@svitlana-galianova looks great. Here's some feedback:

  • There is no need for CollapseAllReplAction, you can use the CollapseAllAction instead
  • I would expect that the Clear action is the last since it is the strongest
  • I would expect a seperator between clear debug console and everything else for nicer visual seperation
@svitlana-galianova

This comment has been minimized.

Contributor

svitlana-galianova commented Feb 20, 2018

I have removed that extra class and moved "Clear Console" option to the end.

collapseall

How can I add a separator? Should it be done in CSS? Maybe you can give me an example of what you are expecting, it would be super helpful, thanks

@isidorn

This comment has been minimized.

Contributor

isidorn commented Feb 21, 2018

@svitlana-galianova

This comment has been minimized.

Contributor

svitlana-galianova commented Feb 21, 2018

So I have added a separator and here is a screenshot 😄

collapseall

@isidorn

This comment has been minimized.

Contributor

isidorn commented Feb 22, 2018

Great work, merging in the pr 🍹

@isidorn isidorn merged commit 0e9649f into Microsoft:master Feb 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment