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

"Collapse unchanged regions" looks disabled #186403

Closed
roblourens opened this issue Jun 27, 2023 · 8 comments · Fixed by #190402
Closed

"Collapse unchanged regions" looks disabled #186403

roblourens opened this issue Jun 27, 2023 · 8 comments · Fixed by #190402
Assignees
Labels
diff-editor Diff editor issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders polish Cleanup and polish issue verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@roblourens
Copy link
Member

roblourens commented Jun 27, 2023

Testing #186213

image

Should use a toggle state instead, or a new icon. You can see it looks the same as the other disabled action

@hediet
Copy link
Member

hediet commented Jun 28, 2023

I agree, but then this also applies to the "toggle whitespace" button.

I think we need some pressed state for toolbar buttons.
What are your thoughts @jrieken @hbons ?

@hediet hediet added polish Cleanup and polish issue diff-editor Diff editor issues labels Jun 28, 2023
@hediet hediet added this to the July 2023 milestone Jun 28, 2023
@jrieken
Copy link
Member

jrieken commented Jul 5, 2023

I'd prefer to have an icon per state. That's how we have done it in other toolbars (like output scroll lock).

@hbons
Copy link
Member

hbons commented Jul 5, 2023

image

Mockup with a toggled state.

Having an icon per state makes sense for the lock icon, because the reverse icon metaphors make intuitive sense. This is not the case for the whitespace and fold icon here though.

cc @daviddossett

@daviddossett
Copy link
Contributor

daviddossett commented Jul 7, 2023

We do use toggles elsewhere. One easy example:

CleanShot 2023-07-07 at 09 13 11@2x

But I think it depends what you're trying to do. Many boolean settings make sense as a sort of enabled/disabled toggle, while others are more specific metaphors (lock/unlock as Hylke mentioned above).

@hediet hediet removed their assignment Jul 12, 2023
@hediet
Copy link
Member

hediet commented Jul 12, 2023

I'm still in favor of a toggle border as we already use this concept in the find widget and it is a universally understood concept, but @jrieken has to agree.
Alternatively, we can explore other icons.
Or we leave it as is.

@jrieken
Copy link
Member

jrieken commented Jul 12, 2023

No, I don't need to agree (but also not to like it). If you wanna explore #186403 (comment) or something similar go ahead but we should be aware that this is also open to extensions, so we need to deal with every (contributed) button looking like this in the future

@hediet hediet self-assigned this Jul 12, 2023
@hediet hediet modified the milestones: July 2023, August 2023 Jul 24, 2023
@abhijit-chikane
Copy link
Contributor

image Mockup with a toggled state.

Having an icon per state makes sense for the lock icon, because the reverse icon metaphors make intuitive sense. This is not the case for the whitespace and fold icon here though.

cc @daviddossett

I think this approach looks good to me
It is more aligned with the vs code current design and should be replaced the current find widget toggle ui

@vscodenpa vscodenpa added unreleased Patch has not yet been released in VS Code Insiders insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels Aug 14, 2023
@hediet hediet added the feature-request Request for new features or functionality label Aug 28, 2023
@hediet
Copy link
Member

hediet commented Aug 28, 2023

Verification steps:

  • Notice that in the diff editor buttons have a background now to indicate their toggled state:
    Code_-_Insiders_Lt8Z0g6xRr

@hediet hediet added the verification-needed Verification of issue is requested label Aug 29, 2023
@jrieken jrieken added the verified Verification succeeded label Aug 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
diff-editor Diff editor issues feature-request Request for new features or functionality insiders-released Patch has been released in VS Code Insiders polish Cleanup and polish issue verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants