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 select all with large response #2694

Merged

Conversation

jgiovaresco
Copy link
Contributor

@jgiovaresco jgiovaresco commented Oct 6, 2020

Closes #2478, Closes INS-921

@jgiovaresco

This comment has been minimized.

@develohpanda
Copy link
Contributor

Hmm, thanks for trying this. Both aren't ideal, one covers text and the other adds a lot of blank space. I'm not quite sure what the best option for this is, yet.

@jgiovaresco
Copy link
Contributor Author

I will try to add an action in the Preview dropdown to copy the response data in the clipboard.

@develohpanda
Copy link
Contributor

develohpanda commented Oct 9, 2020

I will try to add an action in the Preview dropdown to copy the response data in the clipboard.

👍

@nijikokun / @erickrico if you have any feedback here that'd be awesome. Some context in #2478. tl;dr is, the ctrl + a select-all action is quite slow when selecting a large response (due to CodeMirror), so an option is to put a copy-to-clipboard button somewhere, but we are unsure on UI placement. There are some options suggested in the issue as well.

@jgiovaresco jgiovaresco force-pushed the fix/2478-select-all-with-large-response branch from e094be2 to 1a21bbd Compare October 11, 2020 14:25
@jgiovaresco
Copy link
Contributor Author

I've added an action Copy raw response in the Preview dropdown

image

@jgiovaresco jgiovaresco marked this pull request as ready for review October 11, 2020 14:33
@nijikokun
Copy link
Contributor

nijikokun commented Oct 20, 2020

The above, inside the dropdown, seems like a good solution for now. That said, I do fear that it might be hidden and a user will encounter the same issue that was originally raised inside of #2478.

My personal opinion on the placement (to avoid overlap and gaps) is to place it here:

Screen Shot 2020-10-19 at 6 10 28 PM

Or here:

Screen Shot 2020-10-19 at 6 11 20 PM

cc @erickrico

@jgiovaresco jgiovaresco force-pushed the fix/2478-select-all-with-large-response branch from 1a21bbd to 9f451ed Compare October 20, 2020 11:20
@jgiovaresco
Copy link
Contributor Author

It looks like this when the button in the Response pane header

image

@develohpanda
Copy link
Contributor

My two cents:

Placing it in the top header disconnects it from what it relates to. Additionally, the copy button is also quite closely tied to which response viewer is currently shown. If the PDF viewer is shown, the copy button doesn't apply, but it does apply for JSON and raw viewers, for instance.

I don't have high hopes for this, but if it's at all possible to wrap text around the button, should it ever be in a position to overlap, that'd be awesome (see gif here). It will need investigation into CodeMirror to determine whether it is achievable.

@erickrico
Copy link

My two cents:

Placing it in the top header disconnects it from what it relates to. Additionally, the copy button is also quite closely tied to which response viewer is currently shown. If the PDF viewer is shown, the copy button doesn't apply, but it does apply for JSON and raw viewers, for instance.

I don't have high hopes for this, but if it's at all possible to wrap text around the button, should it ever be in a position to overlap, that'd be awesome (see gif here). It will need investigation into CodeMirror to determine whether it is achievable.

i agree, especially if the copy button disappear one you switch to a different tab (header cookies, timeline) i would suggest to keep it under the "preview dropdown"

This commit adds 2 properties in the ResponseViewer state:
* largeResponse which will be true when a response body weight more that LARGE_RESPONSE_MB
* hugeResponse which will be true when a response body weight more that HUGE_RESPONSE_MB
This action allows the user to copy the raw response directly in the clipboard.
It is a useful shortcut to copy huge response where select-all might freeze the application.

Closes Kong#2478
@jgiovaresco jgiovaresco force-pushed the fix/2478-select-all-with-large-response branch from 9f451ed to 5af9d7e Compare October 22, 2020 11:16
@nijikokun
Copy link
Contributor

I agree with the discussions above, let's keep in the dropdown for now. Given that we are also on an older version of codemirror this might be resolved when we upgrade in the future.

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.

We are making an effort to work through our open-source PRs, sorry about the big delays! I updated this branch with develop and QA'd with the test files available here: https://code.google.com/archive/p/jquery-speedtest/downloads.

LGTM! Works as intended. The fact that we can handle/work with 100mb responses with a similar responsiveness (pun intended) to VS Code is promising, and the copy raw response appears to be working well. https://www.loom.com/share/bce854e46d734e98a5ea25d325e50c5a

Thank you for your hard work 🙌🏽

Copy link
Contributor

@gatzjames gatzjames left a comment

Choose a reason for hiding this comment

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

🚀

@develohpanda develohpanda merged commit 4cbd5fa into Kong:develop Sep 9, 2021
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.

Select all text operation is painfully slow with a large response
6 participants