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

Allow ESC to reset UI #29

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Conversation

bryevdv
Copy link
Contributor

@bryevdv bryevdv commented Oct 10, 2023

This PR adds support for having the ESC key reset the UI. cc @lightsighter

Reset includes:

  • clearing controls help
  • clearing profile filter selections
  • clearing any hovers generated by clicking spans

This PR also applies some clippy fixes related to or_default.

cc @elliottslaughter if there is some better approach that does not requires passing the windows list through, please let me know.

@elliottslaughter
Copy link
Contributor

Most of this is fine, but:

clearing profile filter selections

To me it seems unintuitive that this would be controlled by ESC. It's a part of the control UI and not a pop-up window. And we don't touch any other part of the control panel (i.e., this does not implement a "reset to defaults" on the control panel).

What do other people think?

@bryevdv
Copy link
Contributor Author

bryevdv commented Oct 11, 2023

To me it seems unintuitive that this would be controlled by ESC.

I could see it going either way, but this was the explicit request from @lightsighter when I asked about the requirements (via slack)

@elliottslaughter
Copy link
Contributor

@lightsighter can you clarify? I'm surprised that you want ESC to reset kind filters.

@lightsighter
Copy link

@lightsighter can you clarify? I'm surprised that you want ESC to reset kind filters.

I don't think I wanted it to reset the filters. I just wanted the pop-up box to go away. Although it might be nice if nothing is selected and you hit ESC then that clears the filters, but mainly what I want is for ESC to unselect anything I've selected and close any pop-up boxes.

@bryevdv
Copy link
Contributor Author

bryevdv commented Oct 12, 2023

👍 my misunderstanding, I've reverted that part in a2a5129 and also cleared the merge conflict.

@elliottslaughter
Copy link
Contributor

Thanks for the clarification. I think it might be nice to have an option to reset things like filters, but I worry about having it be on ESC because it seems easy to press by accident. But we could potentially have a Reset button on the UI panel itself, or else come up with a key binding that's harder to press by accident (but then it'll be less intuitive as well, so maybe not a win overall).

Anyway, this looks good to me now, so I'll take it once CI passes. Please keep sending these kinds of UI feedback items if there is other stuff that we should consider.

@elliottslaughter elliottslaughter merged commit 6417711 into StanfordLegion:master Oct 12, 2023
6 checks passed
@bryevdv bryevdv deleted the bv/esc-key branch October 12, 2023 18:54
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.

3 participants