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

Collapse panel on toggle view #6963

Merged
merged 2 commits into from
Jan 29, 2020
Merged

Collapse panel on toggle view #6963

merged 2 commits into from
Jan 29, 2020

Conversation

spoenemann
Copy link
Contributor

What it does

Even if I once implemented this behavior myself (#1028), I often find it confusing that the command to toggle a view closes it if it's already visible. And I'm not the only one:
#1727 (comment)
arduino/arduino-pro-ide#188
A side effect of this is that if there are views in the sidebar that have not specified a rank, closing and reopening a view can lead to a different order of icons.

I propose to slightly change this behavior. With this PR, the toggle command collapses the containing panel when the view is visible. The result is similar as before: the view is no longer visible. The difference is that the view icon is still there. If you really want to close the view, you can do that with right-click on its icon > Close.

Special rules for the main and bottom panel:

  • The main panel cannot be collapsed, so if a view is in that area the behavior is the same as before (it is closed).
  • In case the bottom panel is split, i.e. it contains multiple visible widgets, the toggle command does not collapse that panel.

How to test

Use the View menu entries or the respective keybindings to toggle some views.

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

I believe the tests need to be updated as well since the toggle behavior is different now:
https://travis-ci.com/eclipse-theia/theia/jobs/279644821#L5310-L5313

@akosyakov akosyakov added the shell issues related to the core shell label Jan 26, 2020
Signed-off-by: Miro Spönemann <miro.spoenemann@typefox.io>
 - Created a utility function `animationFrame` along with a test to
conveniently await one (or more) animation frame.
 - Fixed a bug in ViewContainerPart where `onBeforeHide` /
`onAfterHide` were not propagated to the wrapped widget unless the part
was collapsed.

Signed-off-by: Miro Spönemann <miro.spoenemann@typefox.io>
@spoenemann
Copy link
Contributor Author

@akosyakov there was a problem in ViewContainerPart: onAfterHide was not propagated to the wrapped widget unless the part was collapsed. Therefore the navigator widget was still marked as visible, which led to the test failure.

https://github.com/eclipse-theia/theia/pull/6963/files#diff-67d5dd89ba01138b8b836376084d0985

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.

cool!

nice that new tests seem to catch actual issues :)

@spoenemann spoenemann merged commit a0d74d3 into master Jan 29, 2020
@spoenemann spoenemann deleted the keep_views_open branch January 29, 2020 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
shell issues related to the core shell
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants