add copyboard to public key #1857
Conversation
AMiracle-creator
commented
May 14, 2024
•
AMiracle-creator
commented
|
|
There was a problem hiding this comment.
Hi! Overall nice work, and quick handling of this recently opened issue! You should sign the CLA otherwise we won't be able to merge your work.
However I think your PR could use some simple tests, as simple as those here.
I also think you should add some screenshots of how the UI looks like, to avoid reviewers to checkout locally :)
Finally, some nitpicks and questions are to be found below!
|
|
||
| private fun setupCopyButton() { | ||
| clipboardManager.setupCopyButton( | ||
| binding.copyServerButton, binding.pkText, "Public Key") |
There was a problem hiding this comment.
Maybe copyServerButton should be renamed in copyPublicKeyButton or something like that
| app:layout_constraintTop_toBottomOf="@id/pk_qr_code" /> | ||
|
|
||
| <ImageButton | ||
| android:id="@+id/copy_server_button" |
There was a problem hiding this comment.
same here, I think you just forgot to adapt the id 👍
| @@ -16,6 +16,7 @@ | |||
| <!-- copy to clipboard --> | |||
| <string name="copy_to_clipboard_server">Copy to Clipboard Server Address</string> | |||
| <string name="copy_to_clipboard_lao_id">Copy to Clipboard Lao ID</string> | |||
| <string name="copy_to_clipboard_public_key">Copy to Public Key</string> | |||
There was a problem hiding this comment.
Just a nitpick but there a typo, should be Copy to Clipboard Public Key
| @@ -294,15 +294,6 @@ | |||
| <string name="popcha_scan_title">Enter PoPCHA URL to log in</string> | |||
| <string name="manual_popcha_hint">PoPCHA URL</string> | |||
|
|
|||
| <!-- Federation --> | |||
There was a problem hiding this comment.
Where do those deletions come from? Maybe a wrong merge? Those strings look like a recent addition from Federation team, why are they getting deleted in this PR?
| @@ -389,4 +376,6 @@ | |||
| <string name="invalid_qrcode_lao_data">Invalid QRCode laoData</string> | |||
| <string name="invalid_qrcode_popcha_data">Invalid URL</string> | |||
|
|
|||
| <string name="network_status_connection_fail">No internet connection</string> | |||
There was a problem hiding this comment.
same for this addition, it looks like it should not be related to this PR
|
Hey, please consider addressing my comments soon. This is a quick fix we do not want to be stuck on :) |
|
@Kaz-ookid Hi, sorry it took me so long to respond. I'll try to fix it soon |
