Disable "Open new terminal" if there's no active workspace#25892
Disable "Open new terminal" if there's no active workspace#25892
Conversation
There was a problem hiding this comment.
I replaced levels.surface as the background of the keyboard shortcut with spotBackground[0].
Initially I wanted to use buttons.bgDisabled since it also uses an alpha channel. But once people start implementing custom themes, they might use a solid color there instead. spotBackground feels more likely to continue to use the alpha channel.
| Before | After |
|---|---|
 |
 |
This will make it easier to show the separator in the story.
ravicious
force-pushed
the
ravicious/new-terminal-no-workspace
branch
from
dc8e93b to
b3b6cb7
Compare
| } & (MenuItemAlwaysEnabled | MenuItemConditionallyDisabled); | ||
|
|
||
| type MenuItemAlwaysEnabled = { isDisabled?: false }; | ||
| type MenuItemConditionallyDisabled = { isDisabled: true; disabledText: string }; |
There was a problem hiding this comment.
I'm trying out something new here. I defined these two explicitly as separate types so that the error message is nicer.
What I wanted to achieve is to force any item that has isDisabled set to true to also specify disabledText.
|
Thoughts on making the icon dimmer on the disabled state? |
|
Thoughts on dimming only the text and the icon (without changing the background)? Similarly to Tags... here: I'm quite overwhelmed by the amount of shadows on the first image 😬 |
|
More like no thoughts, head empty on my part. idk what I was thinking, I was super fixated on using the new colorscheme so for some reason I thought I just had to use another background color. 😅 Dimming the icon + text only should work better, I'll try to update this tomorrow. |
|
This is how it looks after dimming only the text and icon color:
I also made it so that the cursor on |
public-teleport-github-review-bot
Bot
removed the request for review
from gzdunek
|
|
||
| &:disabled { | ||
| cursor: default; | ||
| color: ${props => props.theme.colors.buttons.textDisabled}; |
There was a problem hiding this comment.
WDYT about using props.theme.colors.text.disabled here? Opacity of buttons.textDisabled is 0.3 while for text.disabled it is 0.36 (which should be our default 'disabled' opacity looking at Figma). DevTools shows quite low contrast ratio for that text, it would be nice if we could increase it at least a little.
There was a problem hiding this comment.
Good point, it also makes more sense semantic-wise.
| Before | After |
|---|---|
 |
 |
|
@ravicious See the table below for backport results.
|
4 participants
After logging out from all clusters or before adding the first one, it was possible to open the additional actions menu and select "Open new terminal". Although it didn't do anything, it's better if we just disable that action in that situation.
To represent a disabled item, I used
colors.buttons.bgDisabledandcolors.buttons.textDisabled. Although it kinda makes the disabled action look sort of like the hovered one, I think it adheres to the theme so it should work fine with custom themes.The second item here is disabled, the third one has hover: