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

Find/replace overlay: improve replace toggle button appearance #2337

Conversation

HeikoKlare
Copy link
Contributor

@HeikoKlare HeikoKlare commented Sep 28, 2024

The button to toggle whether the replace bar in a find/replace overlay is shown currently appears to be a rather heavyweight button with a border. With this change, the button is replace with a lightweight toolbar item like used for all other buttons in the overlay. This improves the appearance as well as the implementation as the same implementation patterns can now be applied to all buttons in the overlay.

This is also a preparatory change for #2254 as with that change optmizing the appearance of the button would otherwise become difficult.

How it looks

Windows

Before:
{97308575-0F78-4232-A151-3E93115F274C}
{D7A666D1-B597-46C2-A220-CD829ACBA500}
{FB7A0830-8AAE-4204-B2AC-BE2A9DFCCF3A}
{F2B46CA7-A9FD-4D6C-B2D5-A8C8B0710BB2}

After:
{4C0AFC57-EA34-4582-A506-50D820761F7C}
{5D4F867C-8ACA-44F8-A8E6-D317EE2D3E9D}
{4DC065F8-126A-476C-A732-1677DC81A31C}
{FBCCA0D9-14C9-47DA-8C86-0635C544B538}

MacOS

Before:
image
image
image
image

After:
image
image
image
image

Linux

Before:
image
image
image
image

After:
image
image
image
image

@HeikoKlare
Copy link
Contributor Author

@Wittmaxi what do you think? Is there anything speaking against this change, maybe any reason why this was not the solution in the beginning?

@HeikoKlare HeikoKlare force-pushed the findreplace-togglebutton-appearance branch from 4d28260 to ab370f5 Compare September 28, 2024 09:38
Copy link
Contributor

github-actions bot commented Sep 28, 2024

Test Results

 1 815 files   1 815 suites   1h 34m 18s ⏱️
 7 702 tests  7 474 ✅ 228 💤 0 ❌
24 267 runs  23 520 ✅ 747 💤 0 ❌

Results for commit 21c0c67.

♻️ This comment has been updated with latest results.

@Wittmaxi
Copy link
Contributor

I think this change looks good, improves code quality and makes maintenance easier in the future. Once the tests go green, this can be merged IMHO (is there a reason this is still a draft?)

grafik
Slight caveat: this outline doesn't really feel to me like it belongs there.

@HeikoKlare HeikoKlare marked this pull request as ready for review September 30, 2024 13:46
@HeikoKlare HeikoKlare force-pushed the findreplace-togglebutton-appearance branch from ab370f5 to d2edd9c Compare September 30, 2024 16:05
@HeikoKlare
Copy link
Contributor Author

Slight caveat: this outline doesn't really feel to me like it belongs there.

Do you mean the missing margin to the right or the small size of the button in general? The former is fixed in the recent push. The latter is something I do not have a solution for since toolbars / toolbar items doe seems to be (easily) sizable.

@HeikoKlare HeikoKlare force-pushed the findreplace-togglebutton-appearance branch 2 times, most recently from 982ef62 to 78ef798 Compare October 1, 2024 09:43
The button to toggle whether the replace bar in a find/replace overlay
is shown currently appears to be a rather heavyweight button with a
border. With this change, the button is replace with a lightweight
toolbar item like used for all other buttons in the overlay. This
improves the appearance as well as the implementation as the same
implementation patterns can now be applied to all buttons in the
overlay.
@HeikoKlare HeikoKlare force-pushed the findreplace-togglebutton-appearance branch from 78ef798 to 21c0c67 Compare October 1, 2024 13:12
@HeikoKlare
Copy link
Contributor Author

Let's give this a try.

@HeikoKlare HeikoKlare merged commit 419bb61 into eclipse-platform:master Oct 1, 2024
17 checks passed
@HeikoKlare HeikoKlare deleted the findreplace-togglebutton-appearance branch October 1, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants