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

Hide tab fade until text overflows with sizing-shrink and close-button-left/off (fix #45728) #45815

Merged
merged 2 commits into from Mar 22, 2018

Conversation

Projects
None yet
2 participants
@Dari-K
Copy link
Contributor

Dari-K commented Mar 15, 2018

Moved the padding-right into the label text, which should hide the tab fade while there aren't enough tabs to fill the tab bar (or until a tab gets the .dirty class) #45728

@bpasero bpasero self-assigned this Mar 15, 2018

@@ -186,10 +194,6 @@

/* No Tab Close Button */

.monaco-workbench > .part.editor > .content > .one-editor-silo > .container > .title .tabs-container > .tab.close-button-off {

This comment has been minimized.

@bpasero

bpasero Mar 15, 2018

Member

@Dari-K is that an oversight that you removed this rule for good?

This comment has been minimized.

@Dari-K

Dari-K Mar 15, 2018

Contributor

@bpasero Well the padding-right was moved to another selector so this rule was empty, should I have left the selector with an empty block?

This comment has been minimized.

@bpasero

bpasero Mar 16, 2018

Member

@Dari-K but I am no longer seeing the padding to the right when tab buttons are off? Please check both for sizing fit and shrink

Since this padding is for the ::after element, would it make sense to add the padding to that element if possible?

This comment has been minimized.

@Dari-K

Dari-K Mar 16, 2018

Contributor

@bpasero yeah, you're totally right, and that would make sense, will fix later.
Edit: actually I think putting the padding on ::after meant that the fade was visible even when the text wasn't overflowing, will check later.

This comment has been minimized.

@bpasero

bpasero Mar 16, 2018

Member

@Dari-K ok thanks

This comment has been minimized.

@Dari-K

Dari-K Mar 19, 2018

Contributor

@bpasero so I fixed the problem with the missing padding, but realised that this solution still may not be good enough. The way it works is moving the padding-right to the label text, which collapses, so when the text isn't overflowing the fade is over the empty padding, and when the padding collapses the fade will be over the text. However the padding-right previously didn't collapse when the tab shrank, so this is a change in behaviour.

Do we have a way to tell when the label is overflowing? Or can you think of another way to hide the fade when we shouldn't be showing it?

This comment has been minimized.

@bpasero

bpasero Mar 20, 2018

Member

@Dari-K no we do not, but maybe we could just do this: it seems to work well when tab-close button is on the right and the reason for that is that we reserve some space on the right hand side of the tab for the close button even if the close button is not visible:

image

What if we would reserve some space to the right also when tab-close button is left or off in case tab sizing is shrink? I assume we would not need much more than 5px

This comment has been minimized.

@Dari-K

Dari-K Mar 21, 2018

Contributor

@bpasero I added a pseudo element to reserve 5px on right for sizing shrink with close button left or off like you said. Seems to work well. 👍

@bpasero bpasero added this to the March 2018 milestone Mar 15, 2018

@Dari-K Dari-K force-pushed the Dari-K:tab-fade-45728 branch from 5c10bd7 to c171a6e Mar 21, 2018

@bpasero bpasero merged commit 2d686ce into Microsoft:master Mar 22, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
license/cla All CLA requirements met.
@bpasero

This comment has been minimized.

Copy link
Member

bpasero commented Mar 22, 2018

@Dari-K good stuff 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment