Some text field improvements. #8680
Conversation
|
What do we gain from this? What is the reasoning behind it? |
|
This seems vaguely OK to me, but I'd rather you pull the shared handler out into its own (possibly anonymous) function rather than invoking OnClick of another widget. |
| closeButton.OnClick(); | ||
| else | ||
| { | ||
| filenameInput.Text = null; |
There was a problem hiding this comment.
This seems dangerous to me. In general you want to use "" over null when it comes to UI text, and it looks like this could break if this handler runs twice.
There was a problem hiding this comment.
Yup, always go for string.Empty.
|
I put the shared closing statements in an Action variable and used that.
But assigning an empty string directly is better I guess. |
|
This PR does the following:
|
| player.Stop(); | ||
| Ui.CloseWindow(); | ||
| onExit(); | ||
| }; |
There was a problem hiding this comment.
Style nits: indented one level too far, and newline after the }; please.
|
There are some nice usability improvements here, but there are a few style issues and a couple of bugs that will need to be fixed before we can merge it. Please also split this into individual commits for lobby / settings / asset browser with commit messages that describe what you said above. |
|
I fixed all the issues. |
| chatTextField.OnEscKey = () => | ||
| { | ||
| if (chatTextField.Text.Length == 0) | ||
| closeLobby(); |
There was a problem hiding this comment.
We deliberately chose not to do this because accidentally quitting from a server is a lossy activity, and this makes it far too easy to trigger. Please revert this to the previous behaviour.
Text field takes keyboard focus by default. Esc key resets filter or closes window if text field is empty.
Esc key resets text field to unedited value and yields focus.
Esc key resets text field to unmodified value and yields focus.
|
Looks fine now. 👍 |
Some text field improvements.
This is split from #8118.