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

Spec for Tab Sizing #4104

Merged
merged 2 commits into from
Feb 9, 2021
Merged

Spec for Tab Sizing #4104

merged 2 commits into from
Feb 9, 2021

Conversation

cinnamon-msft
Copy link
Contributor

Summary of the Pull Request

This is the spec for #597

I am proposing the tabWidthMode feature be added first, then tabWidthMin and tabWidthMax be added in a later release.

References

PR: #3876

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

@cinnamon-msft cinnamon-msft added Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Area-Settings Issues related to settings and customizability, for console or terminal labels Jan 3, 2020
@cinnamon-msft cinnamon-msft requested a review from a team January 3, 2020 19:12
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I agree that even should probably be equal, but otherwise this looks good.

Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

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

Just one clarifying question, otherwise looks good!

doc/specs/#597 - Tab Sizing.md Show resolved Hide resolved
@cinnamon-msft cinnamon-msft added the Needs-Second It's a PR that needs another sign-off label Jan 6, 2020
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

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

I mean, it sounds straightforward to me. I don't think it's really high on the priority list of things for us to add, but if you believe there's sufficient motivation... you're the PM so I'm not going to stand in the way.

My comments are non-blocking.

In addition to `tabWidthMode`, the following global properties will also be available:

* `tabWidthMin`
* Accepts an integer
Copy link
Member

Choose a reason for hiding this comment

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

Hopefully they're unsigned integers. Negative minimum and maximum width? Oh boy.

* Accepts an integer
* Value correlates to the minimum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it somewhat weird to say the minimum width of your tab will consume the ENTIRE tab bar? Does that mean with 2 tabs you instantly get the arrows because each tab is an entire width?

@zadjii-msft
Copy link
Member

I realize this is the endgame of the spec, and you've got the signoffs - but is there any way you could maybe enumerate a couple settings examples and how bit the tabs end up being? Case(s) in point:

{
    "tabWidthMode": "equal",
    "tabWidthMax": 300,
    "tabWidthMin": 400
}

and

{
    "tabWidthMode": "equal"
}

What happens with one tab in the second case?

What about the following:

{
    "tabWidthMode": "titleLength"
}

If my tab title is excessively long, will "the system-defined minimum width" cut off my tab title?

@DHowett-MSFT
Copy link
Contributor

Working through my thoughts. I’ll review this when I’m at my desk.

Copy link
Contributor

@DHowett-MSFT DHowett-MSFT left a comment

Choose a reason for hiding this comment

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

I'm still curious about my questions from the original PR.

  • this isn't a width, it is a width mode
  • should we ditch the names equal and sizeToContent? 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 specify 10-300
    • 300 would be shorthand for 300-300 (fixed width)
    • 300-auto would be min=300, max = undefined
    • auto would be shorthand for auto-auto
    • the default would be auto (the same behavior as today)

Also, do our users need so many knobs? Can we just expose a single tabWidth that is fit or shrink or expand or 300? It seems like we're popping out 3 settings when one would do.

How would the settings UI look for this? Would there be one dropdown and two different sliders?

Tab Width Mode
[ Equal       v ]

Tab Minimum Width
-[]-------------- 30

Tab Maximum Width
-------------[]-- 350

it seems . . . cumbersome.

* `equal`
* All tabs are equal in width
* If the tab bar has filled, tabs will shrink as additional tabs are added
* Utilizes the `equal` setting from WinUI's TabView
Copy link
Contributor

Choose a reason for hiding this comment

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

How big is equal if I do not specify sizes?


* `tabWidthMin`
* Accepts an integer
* Value correlates to the minimum amount of pixels the tab width can be
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the value correlate with the # of pixels, or is it just literally the # of pixels? Using correlation here is unusual.

* Accepts an integer
* Value correlates to the minimum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
Copy link
Contributor

Choose a reason for hiding this comment

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

Why must we impose such a restriction/

* Value correlates to the minimum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
* If not set, the tab will have the system-defined minimum width
Copy link
Contributor

Choose a reason for hiding this comment

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

which is?


* `tabWidthMax`
* Accepts an integer
* Value correlates to the maximum amount of pixels the tab width can be
Copy link
Contributor

Choose a reason for hiding this comment

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

does it correlatE?

* If not set, the tab will have the system-defined minimum width

* `tabWidthMax`
* Accepts an integer
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I not specify infinite?

* Value correlates to the maximum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
* If not set, the tab will have the system-defined maximum width
Copy link
Contributor

Choose a reason for hiding this comment

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

which is?

Copy link
Member

Choose a reason for hiding this comment

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

After a bit of testing, looks like XAML doesn't set a max width for us:
really long tab title

We should keep this behavior as default (aka 'infinite').

EDIT: ok, theoretically I could keep going but I'm just assuming there's no max width...

* Accepts an integer
* Value correlates to the maximum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
Copy link
Contributor

Choose a reason for hiding this comment

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

I beg to differ. Why can't I have the maximum width be truly insane if I want it to?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a maximum.

* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
* If not set, the tab will have the system-defined maximum width

Copy link
Contributor

Choose a reason for hiding this comment

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

what if min>max?


## UI/UX Design

[This tweet](https://twitter.com/cinnamon_msft/status/1203094776117022720) displays how the `equal` and `titleLength` values behave for the `tabWidthMode` property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move the screenshot from the tweet into the spec itself. Twitter is not a durable data store.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that animations are painful, it'd be nicer if the individual frames were split out and labeled :-)

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 7, 2020
@cinnamon-msft
Copy link
Contributor Author

Given that the comments are only about the "tabWidthMin" and "tabWidthMax" properties, are we okay with approving #3876 which only implements "tabWidthMode" and addressing the min/max preferences later? We can leave this spec open until everything is resolved :)

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jan 8, 2020
* Value correlates to the maximum amount of pixels the tab width can be
* If value is less than the width of the icon, the minimum width will be the width of the icon
* If value is greater than the width of the tab bar, the maximum width will be the width of the tab bar
* If not set, the tab will have the system-defined maximum width
Copy link
Member

Choose a reason for hiding this comment

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

After a bit of testing, looks like XAML doesn't set a max width for us:
really long tab title

We should keep this behavior as default (aka 'infinite').

EDIT: ok, theoretically I could keep going but I'm just assuming there's no max width...


### Accessibility

This feature could impact accessibility if the tab title isn't stored within the metadata of the tab. If the tab width is the width of the icon, then the title isn't visible. The tab title will have to be accessibile by a screen reader.
Copy link
Member

Choose a reason for hiding this comment

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

The tab title is stored in the tab XAML element itself so you don't have to worry about accessibility. Even for low-vision users, the tooltip serves as a way to display the full tab title.

@zadjii-msft
Copy link
Member

Hey so this spec got to 3 sign-offs, but since @DHowett-MSFT is now @DHowett, it's not technically blocked on his review anymore. We should probably leave this open till we get hist check on it too.

@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:27
@ghost ghost requested review from DHowett and PankajBhojwani October 22, 2020 00:28
@miniksa
Copy link
Member

miniksa commented Feb 9, 2021

This has been hanging out a long time. I asked @DHowett this morning and he said "just merge it". I think it's sufficient for now and we can make a future PR if this needs to evolve further.

@miniksa miniksa merged commit cad7954 into main Feb 9, 2021
@DHowett DHowett deleted the cinnamon/tab-sizing-spec branch October 26, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Settings Issues related to settings and customizability, for console or terminal Issue-Docs It's a documentation issue that really should be on MicrosoftDocs/Console-Docs Needs-Second It's a PR that needs another sign-off
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants