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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent newlines for singleline Code editor #2704

Merged
merged 3 commits into from Oct 13, 2020

Conversation

thewheat
Copy link
Contributor

@thewheat thewheat commented Oct 8, 2020

Aims to fix #2569 by detecting the enter key being pressed and returning early so that it doesn't register as a key press

Behaviour before the change
before

Behaviour after the change
after

Note: There were some other changes when I saved the file/ran the app that I did not include. Am unsure if that was done based on the editor configurations or if I haven't fully configured .editorconfig correctly 馃槄

develohpanda
develohpanda previously approved these changes Oct 8, 2020
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Seems good to me 馃憤

What happens when you paste something with a linebreak? Can look at that in a separate PR though.

Note: There were some other changes when I saved the file/ran the app that I did not include. Am unsure if that was done based on the editor configurations or if I haven't fully configured .editorconfig correctly 馃槄

What were the additional files? With the change you have made I don't anticipate any other changes, but it's possible there were package-lock.json changes (which are okay to leave out).

@develohpanda develohpanda changed the title Closes #2569: Prevent newlines for singleline Code editor Prevent newlines for singleline Code editor Oct 8, 2020
@thewheat
Copy link
Contributor Author

thewheat commented Oct 9, 2020

What were the additional files?

The changes were in the same file I was editing. Digging a bit more I think I got some mix up with the old master branch 馃槄 Using the lastest code it seems that my change doesn't work for the current develop branch. I'll go back to the drawing board with this 馃檲 Sorry!

@develohpanda
Copy link
Contributor

develohpanda commented Oct 9, 2020

No worries! I'll mark this PR as draft, feel free to mark as ready, once you're ready for review again. Take your time, no rush. 馃槃

@develohpanda develohpanda marked this pull request as draft October 9, 2020 04:35
@thewheat thewheat force-pushed the fix-single-line-input-newlines branch from 62dc390 to 03099a7 Compare October 9, 2020 05:20
sonicyeti
sonicyeti previously approved these changes Oct 9, 2020
Copy link
Contributor

@sonicyeti sonicyeti left a comment

Choose a reason for hiding this comment

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

Nice UX enhancement here!

.replace(/\n/g, ' '); // Convert all whitespace to spaces
change.update(change.from, change.to, [text]);
}

// Suppress lint on empty doc or single space exists (default value)
if (value.trim() === '') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A newline value when trimmed will be an empty string so old code to detect multilines is skipped. So just moved code up to allow processing so no net new code 馃挭
image

@thewheat
Copy link
Contributor Author

@develohpanda Managed to clean up my branch and updated code with a version that works with latest develop branch 馃帀
It doesn't add any code but actually just moves existing code around

@thewheat thewheat marked this pull request as ready for review October 10, 2020 09:47
@develohpanda develohpanda dismissed stale reviews from sonicyeti and themself October 11, 2020 08:46

Needs re-review after the fix was changed

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working through this!

// Don't allow non-breaking spaces because they break the GraphQL syntax
if (doc.options.mode === 'graphql' && change.text && change.text.length > 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this condition also needs to be moved out, for the same reason? The change.text.length > 1 condition is the same, and looking through the logic the graphql editor seems like it might exhibit the same buggy behavior when first entering text.

It doesn't relate directly to the issue this PR fixes so I would suggest doing it in a separate PR if it does need to be moved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I'm not familiar with the graphql side of things so it wasn't something I tested. But definitely happy to test it and submit a PR if you know how and when that doc.options.mode == 'graphql' would get triggered 馃槂

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an example workspace with a request containing the special character after the "title" text (see screenshot below).

Run in Insomnia}

I think the intended behavior of this code is to replace the special space with a regular space. If you change the text in the request body, I'm guessing it should clear the error by replacing the character 馃

image

Also, on Windows you can add that special character with Alt 255 (hold alt, and type 255 on the numpad)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Managed to replicate the problem and PR to fix it is here #2770 馃挭

@develohpanda develohpanda merged commit a9e2207 into Kong:develop Oct 13, 2020
1 check passed
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.

Request broken if line breaks inside request auth options fields
4 participants