feat: Added copy text to clipboard from kvm using OCR#1207
feat: Added copy text to clipboard from kvm using OCR#1207tvinhas wants to merge 1 commit intojetkvm:devfrom
Conversation
|
|
adamshiervani
left a comment
adamshiervani
left a comment
There was a problem hiding this comment.
Thanks for taking the time developing this!
| {status === "result" && | ||
| createPortal( | ||
| <div className="pointer-events-none fixed inset-0 z-50 flex items-center justify-center"> | ||
| <div className="pointer-events-auto mx-4 flex max-h-[80%] w-full max-w-md flex-col rounded-lg border border-slate-200 bg-white shadow-xl dark:border-slate-700 dark:bg-slate-800"> |
There was a problem hiding this comment.
Please use <ConfirmDialog>, which uses <Modal> which contains the mounting animations too, so no react motion needed here.
| </button> | ||
| </div> | ||
| <div className="px-4 py-3"> | ||
| <TextArea |
There was a problem hiding this comment.
If possible, make this readonly and highlight all the text. I know Ctrl+C works, but it feels weird if i'm copying a non-selected text.
If you can highlight the text, you don't don't need to the copy hint below
| style={selectionStyle} | ||
| > | ||
| {selectionRect.width >= 10 && selectionRect.height >= 10 && ( | ||
| <div className="absolute right-0 -bottom-6 rounded bg-black/70 px-1.5 py-0.5 text-[10px] font-medium text-white tabular-nums"> |
| rows={Math.min(10, ocrResult.split("\n").length + 1)} | ||
| /> | ||
| </div> | ||
| <div className="border-t border-slate-200 px-4 py-3 dark:border-slate-700"> |
There was a problem hiding this comment.
The <ConfirmDialog> pushes for this, but just as a note - it doesn't have any X, and uses CTA's for the main actions.
I would suggest removing this label, and have two CTAs - Cancel and Copy. They both also get corresponding shortcuts - Esc and CMD+C/ Ctrl+C
| onTouchEnd={handlePointerUp} | ||
| > | ||
| {/* Semi-transparent background */} | ||
| <div className="absolute inset-0 bg-black/20" /> |
There was a problem hiding this comment.
This doesn't go all the way to the border end of the viewport
| )} | ||
|
|
||
| {/* Processing indicator */} | ||
| {status === "processing" && ( |
There was a problem hiding this comment.
The OCR can take some time, this is too tiny to indicate processing. Additionally, users might want to cancel the processing. You could use the same <ConfirmDialog> as the result status, but with a:
- Skelleton Loading with label on top to indicate processing
- Disabled
CopyCTA - onlyCancel
| onClick={() => setTerminalType(terminalType === "kvm" ? "none" : "kvm")} | ||
| /> | ||
| )} | ||
| <Button |
There was a problem hiding this comment.
Note: The Action bar is already too crowded. You don't need to change anything here, but once I get time I'll rethink the action bar, so. just letting you know that I'll probably move after it merges with dev.
| @@ -49,6 +49,7 @@ | |||
| "access_update_tls_settings": "Update TLS Settings", | |||
There was a problem hiding this comment.
Once the Review comments have been addressed make sure to machine translate to other languages too. See DEVELOPMENT.md for details.
2 participants
Summary
Checklist
make test_e2elocally and passedCloses #<issue-number>)