-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 tab width modes: equal and titleLength #3876
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the default yet but the other issue I had is not really worth blocking over.
Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this needs a mini-spec, actually.
Couple thoughts:
- this isn't a width, it is a width mode
- should we ditch the names
equal
andsizeToContent
? We inherited them from the tab control, but are they right for our users? - should we eventually support a multi-state width syntax?
$min - $max
, so I could specify10-300
300
would be shorthand for300-300
(fixed width)300-auto
would be min=300, max = undefinedauto
would be shorthand forauto-auto
- the default would be
auto
(the same behavior as today)
(I'm not asking for a spec because I want these idea to win; I'm asking because if I'm thinking them, others may be too, and that warrants a discussion 😄)
*
|
|
RE But ^ is fairly corner case. Given this particularly precept, when there are a small number of tabs, you could size to content but when those combined tab widths exceed the width of the tab area, then you switch to fixed-width such that all tabs are guaranteed to be visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree with @DHowett-MSFT, I kinda expected a mini-spec laying out the possible states that people would want for this.
I kinda expected that there'd be two settings here: tabMinWidth
and tabMaxWidth
.
tabMinWidth
=tabMaxWidth
=null
(or undefined): the behavior we have now, size to contenttabMinWidth
=tabMaxWidth
=200
(or whatever): the "equal" behavior from this PRtabMinWidth
=null
,tabMaxWidth
=200
: tabs can be arbitrarily small, but capped at 200px- etc.
The only problem I foresaw with that design was the future hypothetical "split" sizing (where tabs take all the space available to them) wasn't really represent-able. I was hoping we'd be able to come up with a combination of settings that would enable all these scenarios.
Plus, now there's @rkeithhill's suggestion, to keep making tabs smaller (to the theoretical tabMinWidth
). Not sure how that's functionally possible with the current TabView (probably isn't). We'd probably want to follow up with them to get support for such a mode.
In the meantime, we'll want to make sure that there's a future-proof design that takes that into account.
The implementation looks fine by me, I kinda think this does need a spec, tbh.
doc/cascadia/SettingsSchema.md
Outdated
@@ -13,6 +13,7 @@ Properties listed below affect the entire window, regardless of the profile sett | |||
| `requestedTheme` | _Required_ | String | `system` | Sets the theme of the application. Possible values: `"light"`, `"dark"`, `"system"` | | |||
| `showTerminalTitleInTitlebar` | _Required_ | Boolean | `true` | When set to `true`, titlebar displays the title of the selected tab. When set to `false`, titlebar displays "Windows Terminal". | | |||
| `showTabsInTitlebar` | Optional | Boolean | `true` | When set to `true`, the tabs are moved into the titlebar and the titlebar disappears. When set to `false`, the titlebar sits above the tabs. | | |||
| `tabWidth` | Optional | String | `equal` | Sets the width of the tabs. Possible values: `"equal"`, `"sizeToContent"` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doc suggests sizeToContent
, though the rest of the pr says sizetocontent
. I'm pretty sure we should go with the camelCase one
I think I'm okay with this being in the root (globals) for now. This is an "application" settings, so it should live with the other application settings. In the future, we might want to consider these properties as being a part of the |
So many people raised issues, I hope equal mode is chosen as the default one |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
shh msftbot, it's the holidays. Take a break. |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
msftbot needs to be made aware of extended holiday breaks. :-) |
This comment represents me seeing this PR but choosing to not review it because there's already 3 cooks in this kitchen (despite the holiday). It serves to remind me of this decision in the future. |
Spec can be found here: #4104 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, there's a spec for the min/max size bits now, so I'm happy with this. I'll unblock it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add this to defaults.json
🎉 Handy links: |
Summary of the Pull Request
Created new global setting "tabWidthMode"
Will accept "equal" or "titleLength"
Equal: Similar to browser experience where all tabs are equal width
TitleLength: Original implementation where width will fit whole tab title
Changed default width behavior to Equal
Refs #597