-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Fixed issue where status bar can overflow without affecting notification beak #155649
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.
Unfortunately this breaks the overflow behaviour of the status bar: the current solution ensures that the notification bell is always the last entry even when we overflow. Now with this solution the notification bell drops first.
I'm having a bit of trouble understanding your suggestion. What is expected when the bell overflows out of the status bar? Or does it never overflow? |
I never overflows. The elements should overflow in this order:
I suggest you just look at the overflow behaviour how it works today, that is the intended overflow behaviour (except for the bug we want to fix). |
In the current version, the leftmost right-hand-side items disappear when resized, and reappear with overflow when tabbing through the status bar. So the bell must always be present at the right end? |
Sorry for not being clear: tabbing through entries is very special in that the browser will forcefully reveal elements. I really don't mind in what order they get revealed, as long as there is no multi-line wrapping. But I do care for how elements overflow normally when not tabbing through the items. That behaviour should be as today, otherwise on small windows we suddenly hide the notification bell for all users. |
So what if we make the bell postion absolute and the rest of the right items can be hidden on overflow and pass through (under) the bell. Now for small windows, some of the right items will be hidden and the bell will always be at the right end(visible) due to absolute positioning. |
No, I do not believe that changing the overflow behaviour is an option here. There can be a lot of people with careful crafted multi-window layouts where then suddenly status bar elements would appear differently. |
So you mean that the overflow must be set visible for the status bar? |
One idea I had was to set An alternative would be to find a way to show the beak always even when |
Ok so, overflow is hidden, beak is showing, bell is showing always, and what will happen to the status bar right items when tabbing? |
Yeah, in the case of beak showing we would still hit this issue. So maybe the best fix is to find a solution for beak showing without having to set |
The current solution shows the beak even with overflow hidden. And I was thinking of making the bell |
To summarise:
|
Things are clearer now, though I still have one question. Suppose the window is so small that only the left-hand items are showing along with the bell on the right. Now if we tab through the entries, where will the right items be displayed? Will the left items push left (overflow) in order to show the right items? |
The clip you shared seems like a good solution. To make the beak display always, we just need to remove |
The overflow behavior is same as earlier and the beak is always showing when the bell is clicked overflow.hidden.with.beak.mp4. |
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.
Unfortunately it is not that easy: the beak can show for any status item, not just the notifications. So it has to be positioned above the status item that enables it.
Ok, so what will happen if somebody clicked the bell and then tabbed to a different line of items? Will the notification box along with the beak disappear and then reappear when the bell is visible? |
The notification box is actually independent from the status entry, but yeah the beak would disappear because it is coming from the status item. |
How about this solution? I made the notification bell always visible using static.notif.bell.mp4 |
Maybe let me try an attempt tomorrow that makes the |
I feel that whenever the overflow becomes visible, then if we tab through the entries, the problem will reappear. |
This is really a though one, tried a lot more things but couldn't come up with something nice. One direction I had thought would be fruitful is to find a way to prevent Summarising again the requirements:
|
This is done only using css and the beak is always displayed over the bell, and the bell is always shown at the far right, other right-hand-items are displayed next to the bell when tabbing. Additionally, when the notification box isn't displayed, the beak isn't displayed as well. And if we hide the status bar from settings, the absolute positioned bell isn't displayed as expected. |
@nirabhromakhal but any item in the status bar can show a beak: Even the most left one |
What are the steps to recreate this issue? I am only able to see the beak on the right. static.notif.bell.mp4 |
@nirabhromakhal we currently do not ship any other status item besides "Notifications" and "Feedback" with a beak that both happen to be on the right hand side. When running locally out of sources, just set vscode/src/vs/workbench/contrib/markers/browser/markers.contribution.ts Lines 458 to 464 in 6246902
|
Okay, there does not seem to be any simple solution to this problem. |
Yeah worth a try 👍 . I think we have freedom how the beak is actually implemented. I wonder if we would still hit the issue with |
Can you check my latest commit? Right now, I have: overflow hidden |
@nirabhromakhal this seems to be working quite well upon first testing, nice 👍 I think your change however reverts some of the changes that meanwhile went into |
If you meant that I need to fetch upstream from microsoft/vscode, I have done it just now. If there is anything else, I'm not sure how to revert. |
.monaco-workbench .part.statusbar > .items-container > .statusbar-item.has-beak { | ||
position: relative; | ||
} | ||
|
||
.monaco-workbench .part.statusbar > .items-container > .statusbar-item.has-beak:before { | ||
.monaco-workbench .part.statusbar > .items-container > .statusbar-item > .beak-div-show { |
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.
Rather than having a beak-div-show
, why not add a CSS class name to the beak container (status-item-beak-container
) and then use the existing has-beak
rule to make this work, e.g.:
.monaco-workbench .part.statusbar > .items-container > .statusbar-item.has-beak > .status-bar-beak-container
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.
yes sure
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 removed a line-break (empty line) by mistake, should I commit again (with the line break)?
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.
src/vs/workbench/browser/parts/statusbar/media/statusbarpart.css
Outdated
Show resolved
Hide resolved
@@ -83,7 +90,7 @@ | |||
} | |||
|
|||
.monaco-workbench .part.statusbar > .items-container > .statusbar-item.right.last-visible-item { | |||
padding-right: 7px; /* Add padding to the most right status bar item */ | |||
margin-right: 7px; /* Add padding to the most right status bar item */ |
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.
Why do we need to change to margin
here?
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.
The centering of the beak is done now based on the width of the status item.
Since the padding is included within the width for the bell, the beak doesn't show at the center for the bell but shows correctly for other status items, unless we change it to margin-right.
Thanks a lot, let's merge this in. I have some 💄 follow up changes that I will push once this lands in |
This PR fixes #154708 with minimal changes to the notification beak.