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

Added Escape Key to CharacterHandler so it can be handled instead of returned. #1981

Closed
wants to merge 16 commits into from
Closed

Conversation

ghost
Copy link

@ghost ghost commented Jul 15, 2019

Summary of the Pull Request

References

PR Checklist

  • [x ] Closes #<ESC> comes through as <ESC><ESC> #1295
  • [ x] CLA signed. If not, go over here and sign the CLA
  • [x ] Tests added/passed
  • Requires documentation to be updated
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

Detailed Description of the Pull Request / Additional comments

Simply, the escape key is not handled, and it should be, as well as adjustments for more clear and maintainable code.

Validation Steps Performed

-Manual testing to ensure no test errors.

@DHowett-MSFT
Copy link
Contributor

Hey @pi1024e, thanks for the contribution. It's generally not great to introduce a bunch of unrelated changes in a single pull request. 😄 We should make sure we comport this with #1974.

@zadjii-msft
Copy link
Member

This looks strictly like a superset of the changes in #1974. I'd rather we take that PR first, then have a discussion if we want to take any of the rest of these changes.

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019
@miniksa
Copy link
Member

miniksa commented Jul 16, 2019

Agreed with the above. I don't want to take this on account of the fact that it's a grab bag of undiscussed things.

If it's updated/submitted with just the one concern at hand, I'm happy to review.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019
@ghost
Copy link
Author

ghost commented Jul 16, 2019

I agree with this! Let us merge #1974 first!

@zadjii-msft
Copy link
Member

Can we get a better title and description of what this PR is supposed to do? It looks like there's a bunch of unrelated changes in a bunch of files, but I have no idea why.

@zadjii-msft zadjii-msft added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 16, 2019
@ghost ghost closed this Jul 17, 2019
@ghost
Copy link
Author

ghost commented Jul 17, 2019

You are right. In fact, I should undo those changes.

@ghost ghost reopened this Jul 17, 2019
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 17, 2019
@ghost
Copy link
Author

ghost commented Jul 17, 2019

Issue has been resolved already in another PR, so I am going to close this

@ghost ghost closed this Jul 17, 2019
This pull request was closed.
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.

5 participants