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

fix inputs not blurring when pressing enter #4692

Merged
merged 4 commits into from
Jan 19, 2022

Conversation

ndrscodes
Copy link
Contributor

Fixes #4669.

This fix/feature causes inputs to blur (causing the onChange event to be triggered) once the Enter key is pressed. As of now, the user has to manually click outside of the active input in order to save the value.

Signed-off-by: Andreas Schmidt <ndrscodes@gmail.com>
@ndrscodes ndrscodes marked this pull request as ready for review January 14, 2022 14:36
@ndrscodes ndrscodes requested a review from a team as a code owner January 14, 2022 14:36
@ndrscodes ndrscodes requested review from jakolehm and Nokel81 and removed request for a team January 14, 2022 14:36
@Nokel81 Nokel81 added area/ui bug Something isn't working labels Jan 14, 2022
@@ -283,6 +283,9 @@ export class Input extends React.Component<InputProps, State> {
} else {
this.setDirty();
}

//pressing enter indicates that the edit is complete, we can unfocus now
this.blur();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this happen a few lines up, after the this.setState() call?

Copy link
Contributor Author

@ndrscodes ndrscodes Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends. doing it like this, the user can unfocus the input even if it's not valid. If we blurred the input right after calling setState, the user would lose the ability to do so (even if trying to submit invalid data doesn't make much sense).. Idk which solution is better.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with components like EditableList? Have you tried that?

makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've tested EditableLists using the accessible namespaces. The namespace is added and after that, the input blurs. Should we keep the input focussed in those cases?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the cleanest way of doing this in this scenario? Checking the type of Input in the onKeyDown method seems pretty unclean, but that would mean each class would have to decide if the input needs to be blurred itself, or needs to focus the input again. All of these solutions are not optimal imho

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I think EditableList is the only place where this matters I would make a prop called something like blurOnEnter with default value true and then EditableList can set it to false.

editable-list.tsx Outdated Show resolved Hide resolved
Signed-off-by: ndrscodes <ndrscodes@gmail.com>
Nokel81
Nokel81 previously approved these changes Jan 14, 2022
Signed-off-by: ndrscodes <ndrscodes@gmail.com>
@ndrscodes
Copy link
Contributor Author

ndrscodes commented Jan 14, 2022

sorry for causing trouble again. I removed an unnecessary call to stopPropagation(). @Nokel81

Nokel81
Nokel81 previously approved these changes Jan 14, 2022
jim-docker
jim-docker previously approved these changes Jan 17, 2022
@Nokel81
Copy link
Collaborator

Nokel81 commented Jan 17, 2022

@ndrscodes Can you please investigate the namespace selector test that is failing?

@ndrscodes
Copy link
Contributor Author

@ndrscodes Can you please investigate the namespace selector test that is failing?

I've checked it, but i do not know what is causing this issue. It seems like the event is not being propagated to the Wizard/WizardStep. As soon as i implement this.blur() (even this.input.blur()), the WizardStep stops executing the next() function. Do you know what could be causing this? @Nokel81

@ndrscodes
Copy link
Contributor Author

@ndrscodes Can you please investigate the namespace selector test that is failing?

I've checked it, but i do not know what is causing this issue. It seems like the event is not being propagated to the Wizard/WizardStep. As soon as i implement this.blur() (even this.input.blur()), the WizardStep stops executing the next() function. Do you know what could be causing this? @Nokel81

What we could do is giving an onKeyDown handler to the WizardStep in order to execute submit when pressing enter. That would also likely lead to cleaner implementation than trusting in onSubmit to fire if Enter is pressed.

@Nokel81
Copy link
Collaborator

Nokel81 commented Jan 17, 2022

What we could do is giving an onKeyDown handler to the WizardStep in order to execute submit when pressing enter. That would also likely lead to cleaner implementation than trusting in onSubmit to fire if Enter is pressed.

That is a good idea

Signed-off-by: ndrscodes <ndrscodes@gmail.com>
Copy link
Collaborator

@Nokel81 Nokel81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks!

@Nokel81 Nokel81 added this to the 5.4.0 milestone Jan 19, 2022
@Nokel81 Nokel81 merged commit 86f14a9 into lensapp:master Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow saving cluster name by tapping esc key while on cluster name text box
4 participants