-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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 support to show close button for active tab only. #15237
Add support to show close button for active tab only. #15237
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.
Yep that's literally how I'd do it 😝 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.
@kovdu I'm not going to merge this yet to give you some time to apply/respond to my comments, but the PR is good to go! Again, thanks for the contribution! |
Thanks for the feedback @carlos-zamora . Fixed your remarks, updated Also verified the new value is correctly applied when hot reloading the settings file. |
This MR introduces
activeOnly
for theshowCloseButton
theme option causing the close button only to appear on the active tab.This is more or less following the approach explained here https://github.com/orgs/microsoft/projects/686/views/2?pane=issue&itemId=19775774 which indeed just works 😄 .
You notice when switching theme the close buttons is back on all tabs again as well.
Closes #13672
I didn't check specific unit tests for this. I hope by making this MR the pipeline will show if I broke something. Or just let me know if you want me to add something specific for this.