Enhance Invite Section with Copy Functionality for LAO Details #1797
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
There was a problem hiding this comment.
Good job! Just few comments.
General suggestion: whenever you're modifying the UI (either layout or button behaviour) it would be nice to add to the PR description screenshots or screen recordings. That would help who's reviewing the PR as they wouldn't need to pull the code and run it to see the changes.
[EDIT: While I was reviewing it you actually anticipated my comment and added a screenshot, I'd call that telepathy :) )
| setupCopyServerButton(binding) | ||
| setupCopyIdentifierButton(binding) |
There was a problem hiding this comment.
I'd save the binding as a class attribute to avoid passing it around (it's just a preference but also how it's usually done in the codebase)
| private fun copyTextToClipboard(token: String) { | ||
| val clipboard = | ||
| requireActivity().getSystemService(Context.CLIPBOARD_SERVICE) as ClipboardManager | ||
| val clip = ClipData.newPlainText(token, token) |
There was a problem hiding this comment.
Here reading the doc the first param should be the text label that you're copying. So instead of reusing the token as its own label, I'd put an actual label that explains what has been copied (not its content)
There was a problem hiding this comment.
yes that's what I did first, but then I saw line 100 in TokenFragment.kt and went to do the same. Maybe I just should have trusted my critical sense 🤷♀️
And I should have changed the argument name, because "token" is not accurate at all
Also, this just makes code duplication through different fragments. Maybe we should put the copyTextToClipboard function somewhere to be used by multiple fragments?
There was a problem hiding this comment.
Yes in an utils file would be perfect
| private fun setupCopyServerButton(binding: InviteFragmentBinding) { | ||
| val serverTextView = binding.laoPropertiesServerText | ||
| val copyButton = binding.copyServerButton | ||
|
|
||
| copyButton.setOnClickListener { | ||
| val text = serverTextView.text.toString() | ||
| copyTextToClipboard(text) | ||
| Toast.makeText(requireContext(), R.string.successful_copy, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } | ||
|
|
||
| private fun setupCopyIdentifierButton(binding: InviteFragmentBinding) { | ||
| val identifierTextView = binding.laoPropertiesIdentifierText | ||
| val copyButton = binding.copyIdentifierButton | ||
|
|
||
| copyButton.setOnClickListener { | ||
| val text = identifierTextView.text.toString() | ||
| copyTextToClipboard(text) | ||
| Toast.makeText(requireContext(), R.string.successful_copy, Toast.LENGTH_SHORT).show() | ||
| } | ||
| } |
There was a problem hiding this comment.
Refactoring opportunity: these 2 methods are almost the same, so you can extract a single method, which is more generic, and parametrise its usage to reduce code duplication
…gful + added binding as class attribute
|
|
|
|
|
Description
This pull request addresses issue #1793 by introducing a copy button next to the LAO ID and server address within the invite section. Additionally, the LAO ID and server address fields have been made selectable for manual copying.
Changes
Impact