-
Notifications
You must be signed in to change notification settings - Fork 34
Remove page with Ctrl+right click on close button #529
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
base: master
Are you sure you want to change the base?
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.
Thanks for this contribution. The change looks good to me.
Please add a changelog entry if possible, otherwise we can do it as well
|
Changelog filled |
|
|
||
| return evt; | ||
| } | ||
| public static bool WithTooltipConditional(this ButtonEvent evt, bool condition, ImGui gui, string tooltip, Rect? rect = null) { |
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.
This function duplicates the code of WithTooltip, with the only difference of an in-built check for boolean. This function has only one usage.
Why not just check this boolean condition in a short-circuiting if before deciding if you want to show the tooltip?
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.
Hmm, I start to understand why, but I wonder if there's a better way. Will look into that.
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.
Something like the following to prevent code duplication. What do you guys think?
if (condition) {
return WithTooltip(evt, gui, tooltip, rect);
}
else {
return evt;
}
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.
Hiding this logic in the new function is cleaner to me, as it does not have this logic all over the place and can directly returned.
So we'd get a function bool WithTooltipConditional that returns evt
|
In general, I try to prevent overloading UI with the QoL functionality that is rarely used. Do you really delete the pages so often that you need a shortcut for that? |
I was wondering the same, but then I figured that most users won't be aware as only the tooltip points to this. So it is not really cluttering the UI I thought... |
It's another ever-appearing popup. This time on a closing button. |
|
I just tried, it is indeed 'funny' to see that a closing button closes the page... Maybe adding this info to the tips of the popup is better yes. <Unrelated> As a non-native English speaker: isn't a tip something you give you waiter? (funny side note: In Dutch a 'tip' has this intended meaning for being a piece of info you want to provide to someone.) If so, we could make another 'good first issue' to fix the naming of this file to the correct wording. |
"tip" also has another meaning - "hint". (proof) |
This pull request adds conditional tooltip support for the tab close button and improves user feedback and interaction when closing tabs in the main screen tab bar. The most important changes are grouped below:
UI/UX Improvements:
WithTooltipConditionalextension method toButtonEventinImGuiUtils.cs, allowing tooltips to be shown only when a specified condition is met.MainScreenTabBar.csto use the new conditional tooltip. The tooltip is now displayed only if the page can be deleted, enhancing clarity for users.Localization and Feedback:
tooltip-close-pageinyafc.cfg, which provides a tooltip message for the close button, including Ctrl+Click instructions for removal.Tab Closing Behavior: