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

Add "open tabs" drop down #12411

Merged
merged 5 commits into from
May 2, 2023

Conversation

tsmaeder
Copy link
Contributor

What it does

Adds a drop down allowing to chose among open editors when there is too little space to show all open tabs. Also adds a fade-out effect for tab labels.

This is the second part of #12328

Contributed on behalf of STMicroelectronics

How to test

Make sure the scenarios from #12360 are still working O.K. Additionally, make sure the drop-down for the open editors appears and disappears in in the proper cases.

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@tsmaeder do you mind resolving the conflicts?

@tsmaeder
Copy link
Contributor Author

Hmh....something is broken. Stand by...

@tsmaeder tsmaeder marked this pull request as draft April 13, 2023 15:39
@tsmaeder
Copy link
Contributor Author

Apparently, some stuff was broken when rebasing on the changes in the previous "dynamic tabs" PR. I'll fix and reopen the PR.

@tsmaeder tsmaeder marked this pull request as ready for review April 14, 2023 09:37
@tsmaeder
Copy link
Contributor Author

Now ready for review.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Unfortunately this change seems to break the default tab behavior.
This is with workbench.tab.shrinkToFit.enabled set to false:

weird-tabs.mp4

In this case the dynamic tab styling should not be applied, and should behave like before.

@@ -55,6 +55,7 @@ test.describe('Theia Terminal View', () => {

// close all terminals
for (const terminal of allTerminals) {
await terminal.activate(); // close box is not visible when not active
Copy link
Member

Choose a reason for hiding this comment

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

minor: do we mean the close icon? I'm not aware of the "close box" which makes the documentation confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the term "close box" is quite common for windows (at least Google thinks so). I had "close button" before, but the UI element is not really a button. An "Icon" feels more like describing the implementation, not the UI element, i.e. the "close icon" is the graphic element, whereas Windows used to have a minus-sign in its window close box.
If you still find it confusing, I can change it.

Copy link
Member

Choose a reason for hiding this comment

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

I believe we should stick with button (which I feel is more common) if we are to keep the comment else I'd remove it completely. I'm leaning towards removing the comment altogether, especially since theia-text-editor.test.ts which has a similar change to this one does not include one.

@tsmaeder
Copy link
Contributor Author

Unfortunately this change seems to break the default tab behavior.

Thanks for catching this one: I am very bad at noticing visual details 🤷

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Unfortunately I've noticed a few regressions with this pull-request:

  1. it is no longer possible to drag tabs and create split layouts
  2. the bottom panel is incorrectly affected by the dynamic tab behaviour, is this intended?

@@ -55,6 +55,7 @@ test.describe('Theia Terminal View', () => {

// close all terminals
for (const terminal of allTerminals) {
await terminal.activate(); // close box is not visible when not active
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should stick with button (which I feel is more common) if we are to keep the comment else I'd remove it completely. I'm leaning towards removing the comment altogether, especially since theia-text-editor.test.ts which has a similar change to this one does not include one.

packages/core/src/browser/shell/tab-bars.ts Show resolved Hide resolved
packages/core/src/browser/style/tabs.css Show resolved Hide resolved
packages/core/src/browser/style/tabs.css Outdated Show resolved Hide resolved
packages/core/src/browser/style/tabs.css Outdated Show resolved Hide resolved
@tsmaeder
Copy link
Contributor Author

The tab behaviour in the lower pane is intended. Having a look at the drag behaviour.

@vince-fugnitto vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023
Adds a drop down allowing to chose among open editors when there is too
little space to show all open tabs.

Part of eclipse-theia#12328

Contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <[email protected]>
Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder
Copy link
Contributor Author

tsmaeder commented May 1, 2023

@vince-fugnitto I believe I got the scroll/resize behavior to be correct now and addressed your comments. Thanks for your patience with this PR.

@vince-fugnitto vince-fugnitto added the ui/ux issues related to user interface / user experience label May 1, 2023
@vince-fugnitto vince-fugnitto dismissed their stale review May 1, 2023 13:51

fix: regression when dragging tabs has been fixed

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍

  • the "open tabs" dropdown is correctly rendered, and displays the tabs for a given group
  • there are no regressions with the default behavior
  • there are no regressions with drag-and-drop to create special layouts

@MatthewKhouzam
Copy link

Could we have a video of the feature? It can help document it.

@tsmaeder tsmaeder merged commit 53c4142 into eclipse-theia:master May 2, 2023
@tsmaeder
Copy link
Contributor Author

tsmaeder commented May 2, 2023

@MatthewKhouzam yes we can!

ShirinkingTabs.mp4

@tsmaeder
Copy link
Contributor Author

tsmaeder commented May 2, 2023

@vince-fugnitto I found another bug when making the video: #12477

@tsmaeder tsmaeder mentioned this pull request May 2, 2023
1 task
@tsmaeder tsmaeder mentioned this pull request May 11, 2023
11 tasks
tsmaeder added a commit to tsmaeder/theia that referenced this pull request May 23, 2023
Adds a drop down allowing to chose among open editors when there is too
little space to show all open tabs.

Part of eclipse-theia#12328

Contributed on behalf of STMicroelectronics
@vince-fugnitto vince-fugnitto added this to the 1.38.0 milestone May 25, 2023
@wpaul-samsung
Copy link

What's the reason for only showing the open editors icon when shrink to fit is enabled and the window width is very narrow?

image
Shrink to fit enabled, small window, no icon.

image
A but smaller than the previous screen shot and now we get the icon.

Notice how the open settings icon is always there. Why don't we just always show the open editors icon to the left of the open settings icon? This is more of less what vscode does BTW.

image
Shrink to fit disabled - no way to see open editors.

@msujew
Copy link
Member

msujew commented Oct 16, 2023

@wpaul-samsung The reason probably was "it's out of scope of the PR". I would recommend you to open a new feature request to facillitate an actionable discussion.

@wpaul-samsung
Copy link

@wpaul-samsung The reason probably was "it's out of scope of the PR". I would recommend you to open a new feature request to facillitate an actionable discussion.

Fair enough.

@tsmaeder
Copy link
Contributor Author

Not sure if I remember this correctly, but I think the "regular" case is also reusing the default tab behavior from PhosphorJS, so changing anything about that might have been hard. But also: "out of scope"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants