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

Convert paste-overlay to paste-dialog #1603

Merged
merged 9 commits into from Aug 31, 2023
Merged

Convert paste-overlay to paste-dialog #1603

merged 9 commits into from Aug 31, 2023

Conversation

jdeanwallace
Copy link
Contributor

@jdeanwallace jdeanwallace commented Aug 30, 2023

Resolves #1137

This PR completely removes the old "Paste" frontend code and replaces it with a custom <paste-dialog> element.

Screen.Recording.2023-08-31.at.14.52.44.mov

Notes:

  1. Once the paste-dialog is open, you can just press the "Enter" key to actually paste it to the target machine. This is in accordance to our style guide:
    If there is a call-to-action button associated with an input field,
    then pressing the “Enter”/“Return” key on the keyboard should trigger
    the call-to-action. The behaviour should be the same as if the button

    However, this behavior might be unexpected especially if you're manually typing text into the textarea and want to add a newline character. With this in mind, I made sure that you can still add a newline character using Shift+Enter.

Review on CodeApprove

@jdeanwallace jdeanwallace changed the base branch from rate-limited-keystrokes to master August 31, 2023 11:53
@jdeanwallace jdeanwallace marked this pull request as ready for review August 31, 2023 12:56
Copy link
Contributor Author

Automated comment from CodeApprove ➜

@mtlynch please review this Pull Request

Copy link
Collaborator

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

Cool, I didn't realize how much complexity we'd eliminate by getting rid of the legacy overlay.


In: Discussion

Once the paste-dialog is open, you can just press the "Enter" key to actually paste it to the target machine. This is in accordance to our style guide:

If there is a call-to-action button associated with an input field,
then pressing the “Enter”/“Return” key on the keyboard should trigger
the call-to-action. The behaviour should be the same as if the button

However, this behavior might be unexpected especially if you're manually typing text into the textarea and want to add a newline character. With this in mind, I made sure that you can still add a newline character using Shift+Enter.

I think this is the right behavior, but we shouldn't forward the <Enter> keystroke to the target system. Currently, it gets sent after the pasted text. For example, if I type hi<Enter>, the target system sees:

2023-08-31 09:16:22.134 socket_api      DEBUG [SENSITIVE] received keystroke message: {'metaLeft': False, 'metaRight': False, 'altLeft': False, 'altRight': False, 'shiftLeft': False, 'shiftRight': False, 'ctrlLeft': False, 'ctrlRight': False, 'key': 'h', 'code': 'KeyH'} [/SENSITIVE]
2023-08-31 09:16:22.154 socket_api      DEBUG [SENSITIVE] received keystroke message: {'metaLeft': False, 'metaRight': False, 'altLeft': False, 'altRight': False, 'shiftLeft': False, 'shiftRight': False, 'ctrlLeft': False, 'ctrlRight': False, 'key': 'i', 'code': 'KeyI'} [/SENSITIVE]
2023-08-31 09:16:22.174 socket_api      DEBUG [SENSITIVE] received keystroke message: {'metaLeft': False, 'metaRight': False, 'altLeft': False, 'altRight': False, 'shiftLeft': False, 'shiftRight': False, 'ctrlLeft': False, 'ctrlRight': False, 'key': 'Enter', 'code': 'Enter'} [/SENSITIVE]

In: Discussion
Can we disable the paste button when the textarea contains an empty string?


In: app/templates/custom-elements/paste-dialog.html:

> Line 17
  <h3>Paste Text to the Target System</h3>

Can we simplify this to "Paste Text"?


In: app/templates/custom-elements/paste-dialog.html:

> Line 19
    The target system needs to have an <i>en-US</i> or <i>en-GB</i> keyboard

Let's change this to:

<p>
  Note: The target system must have an <span class="monospace">en-US</span> or
  <span class="monospace">en-GB</span> keyboard layout.
</p>

In: app/templates/custom-elements/paste-dialog.html:

> Line 50
            pasteArea: this.shadowRoot.querySelector("#paste-area"),

(nit) Suggest reordering either alphabetically or by order of appearance (top to bottom, left to right).


👀 @jdeanwallace it's your turn please take a look

Copy link
Contributor Author

@jdeanwallace jdeanwallace left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

In: Discussion
Sure.


In: Discussion
Oh, nice catch. Fixed.


In: app/templates/custom-elements/paste-dialog.html:
Done.


In: app/templates/custom-elements/paste-dialog.html:
Sure.


In: app/templates/custom-elements/paste-dialog.html:
Thanks, I went with order of appearance.


👀 @mtlynch it's your turn please take a look

Copy link
Collaborator

@mtlynch mtlynch left a comment

Choose a reason for hiding this comment

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

Automated comment from CodeApprove ➜

Approved on CodeApprove
✔️ Approved

LGTM, thanks!


👀 @jdeanwallace it's your turn please take a look

@jdeanwallace jdeanwallace merged commit 325d56f into master Aug 31, 2023
14 checks passed
@jdeanwallace jdeanwallace deleted the paste-overlay branch August 31, 2023 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert paste overlay to a dialog box
2 participants