KLO-227 : Copy button is not giving feedback of copying. #146
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtCoder19 - I've reviewed your changes and they look great!
General suggestions:
- Ensure consistent user feedback for repeated copy actions within short intervals.
- Consider the impact of icon size changes on the overall UI design and alignment.
- Review the necessity of conditional checks before executing copy actions to avoid potential user confusion.
- Remove commented-out code that is no longer needed to maintain code cleanliness.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| const handleCopy = () => { | ||
| copy(url); | ||
| setCopied(true); | ||
| toast.success('Registry url copied successfully.'); | ||
|
|
||
| setTimeout(() => { | ||
| setCopied(false); | ||
| }, 3000); | ||
| }; |
There was a problem hiding this comment.
suggestion (code_refinement): The handleCopy function introduces a 3-second delay before resetting the copied state to false. While this works for visual feedback, consider the user experience if the user attempts to copy the URL again within those 3 seconds. The current implementation will not provide feedback for the second copy action. It might be beneficial to allow the toast message to be triggered on every copy action, regardless of the copied state, to ensure the user receives consistent feedback.
| if (!copied) { | ||
| handleCopy(); | ||
| } |
There was a problem hiding this comment.
issue (code_refinement): The conditional check if (!copied) before calling handleCopy might lead to a scenario where the user cannot copy the URL again until the copied state is reset. This could be confusing if the user wants to copy the URL multiple times in quick succession. Consider removing this condition to allow for a more flexible user experience.
| @@ -82,25 +82,27 @@ export const CopyButton = ({ | |||
| title: ReactNode; | |||
| value: string; | |||
| }) => { | |||
| const [_, setCopyIcon] = useState(<CopySimple />); | |||
| // const [_, setCopyIcon] = useState(<CopySimple />); | |||
| const [copied, setCopied] = useState(false); | |||
There was a problem hiding this comment.
suggestion (code_refinement): The introduction of the copied state to manage the icon change in CopyButton is a good enhancement for user feedback. However, ensure that the onSuccess callback of useClipboard is properly handling the case where the copy action might fail for any reason. Currently, the copied state is only set to false after a delay, without considering the success of the copy action.
| @@ -82,25 +82,27 @@ | |||
| title: ReactNode; | |||
| value: string; | |||
| }) => { | |||
| const [_, setCopyIcon] = useState(<CopySimple />); | |||
There was a problem hiding this comment.
nitpick (code_refinement): Commenting out the previous state declaration without removing it indicates a transition phase or an uncertainty about the removal. If the copied state fully replaces the need for setCopyIcon, it's cleaner to remove the commented-out code to avoid confusion and maintain code cleanliness.
| copy(value); | ||
| }} | ||
| className="flex flex-row gap-md items-center select-none group cursor-pointer" | ||
| > | ||
| <span>{title}</span> | ||
| <span className="invisible group-hover:visible"> | ||
| <CopySimple size={10} /> | ||
| {copied ? <Check size={12} /> : <CopySimple size={12} />} |
There was a problem hiding this comment.
suggestion (code_refinement): The change in icon size from the default to 12 for both Check and CopySimple icons is a subtle but important UI detail. Ensure that this size change integrates well with the overall design and doesn't cause any alignment or scaling issues, especially when the icon state changes from not copied to copied.
nxtCoder19
force-pushed
the
update/copy-button
branch
from
8f43dae to
8c9ad84
Compare
KLO-227 : Copy button is not giving feedback of copying.
KLO-227 : Copy button is not giving feedback of copying.
KLO-227 : Copy button is not giving feedback of copying.
Resolves kloudlite/kloudlite#66
Attachment