-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
focus out possible again for debug configuration #12046
focus out possible again for debug configuration #12046
Conversation
Why do I feel like this change replaces a hack with another hack? Is there a React API to listen for the dropdown closing rather than using |
yeah it really is a little bit hacky. I searched a little, but could't find anything on closing the dropdown and defocussing the selectComponent seems to also not fix the issue. |
@martin-fleck-at we should decide if the "hack" is a step forward and either merge or close the PR accordingly. |
@tsmaeder Please excuse my delayed response, I'll have a look at this later this week. |
@@ -103,7 +103,7 @@ export class DebugConfigurationSelect extends React.Component<DebugConfiguration | |||
if (!value) { | |||
return false; | |||
} else if (value === DebugConfigurationSelect.ADD_CONFIGURATION) { | |||
this.manager.addConfiguration(); | |||
setTimeout(() => this.manager.addConfiguration()); |
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.
I agree that this is a bit 'hacky' and I tested animationFrame().then(() => this.manager.addConfiguration());
which also seems to work but I don't see any additional benefit from using that instead. I also did not find any alternative solution that solves that issue but I also don't think there are going to be any side effects from this, so in my opinion this could be merged.
The only thing that would be nice is a short comment explaining why this particular call needs to be handled differently though in theory that could be derived from the Git history. @tsmaeder How do you usually handle these situations with regard to documentation?
@msujew Do you want to merge this? |
workspace quick pick Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
ec886b6
to
42a5230
Compare
What it does
fixes #11992
this makes the call to configurationManager.addConfiguration() execute after react has closed the dropdown. Through this it's possible to remove the prevous ignoreFocusOut fix and make it possible again to click outside of the quick select to cancel it.
How to test
Review checklist
Reminder for reviewers