-
Notifications
You must be signed in to change notification settings - Fork 193
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
Add close button to find/replace overlay #1991 #2032
Conversation
Adds an icon for closing the find/replace overlay. See eclipse-platform/eclipse.platform.ui#2032
Test Results 1 815 files + 2 1 815 suites +2 1h 33m 40s ⏱️ - 2m 59s For more details on these failures, see this check. Results for commit 05f134e. ± Comparison against base commit 80c6da9. ♻️ This comment has been updated with latest results. |
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.
Looks good to me.
That would clearly separate the single closing operation from the preceding search operations, but in my opinion it bloats up the overlay providing a significant benefit.
Agree on that. On first sight it would be nice to separate it, but since all buttons on the right of the first separator are some kind of control buttons, I think it is fine to group them all together.
Ia agree that we shouldn't separate the closing-button, a separator would just add more to visually process and does not add any clear benefit. Code looks sound! |
I have documented the second part of the issue (which was only documented in the trace of comments below) |
Thank you for your quick feedback! |
Look great, thanks |
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.
Looks good and manually test on macOS dark theme is fine.
This new close icon can also be used to update the ugly notification close API. See #2035 |
The find/replace overlay can currently only be closed via pressing Escape or using the shortcut CTRL+F when the overlay has input focus. This change adds a close button to the overlay for providing an easy and comprehensible way of closing the overlay. Contributes to eclipse-platform#1991
I consider your positive feedback as a general approval, so I will merge this PR now. In case there are things to change/optimize, we can still do that as a follow-up, but I guess it's good to have an easy way to close the overlay provided as soon as possible. The failing test is a known random failure (#926) and the freeze period check is just because of M1 promotion, which does not forbid merges anymore. |
Adds an icon for closing the find/replace overlay. See eclipse-platform/eclipse.platform.ui#2032
The find/replace overlay can currently only be closed via pressing Escape or using the shortcut CTRL+F when the overlay has input focus. This change adds a close button to the overlay for providing an easy and comprehensible way of closing the overlay.
In order to be compatible with different themes on all OS, we can, unfortunately, not reuse some shared close icons (i.e., one already provided in the platform.ui plugins). The icon adopts the color scheme (i.e., the same shade of gray) of the existing search options buttons.
Contributes to #1991
How it looks like (Windows)
Alternatives
As an alternative, this is how it looks like with a preceding separator.
That would clearly separate the single closing operation from the preceding search operations, but in my opinion it bloats up the overlay providing a significant benefit. Still it would be great to have our opinions (on the alternative as well as the proposal in general).
PR for images repository
The images (including the orignal SVG) are provided here: eclipse-platform/eclipse.platform.images#80
Relation to other PRs
If this is merged before #2030, that PR will probably need some adaptation for the considerations of the toolbars on size calculation.