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.

Sign up for GitHub

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

KLO-227 : Copy button is not giving feedback of copying. #146

Merged
merged 2 commits into from
Mar 14, 2024
Merged

KLO-227 : Copy button is not giving feedback of copying. #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f103b6a
update copy button
nxtCoder19 Mar 14, 2024
8c9ad84
update copy button
nxtCoder19 Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/apps/console/components/commons.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CopySimple, Question } from '@jengaicons/react';
import { CopySimple, Question, Check } from '@jengaicons/react';
import { ReactNode, useState } from 'react';
import { ProdLogo } from '~/components/branding/prod-logo';
import { WorkspacesLogo } from '~/components/branding/workspace-logo';
Expand Down Expand Up @@ -82,25 +82,27 @@ export const CopyButton = ({
title: ReactNode;
value: string;
}) => {
const [_, setCopyIcon] = useState(<CopySimple />);
// const [_, setCopyIcon] = useState(<CopySimple />);
Copy link

Choose a reason for hiding this comment

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

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.

const [copied, setCopied] = useState(false);
const { copy } = useClipboard({
Copy link

Choose a reason for hiding this comment

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

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.

onSuccess: () => {
setTimeout(() => {
setCopyIcon(<CopySimple />);
setCopied(false);
}, 1000);
},
});

return (
<div
onClick={() => {
setCopied(true);
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} />}
</span>
Copy link

Choose a reason for hiding this comment

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

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.

</div>
);
Expand Down
36 changes: 26 additions & 10 deletions src/apps/console/routes/_main+/$account+/packages+/repos/repo-resources.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Copy, Trash } from '@jengaicons/react';
import { Copy, Trash, Check } from '@jengaicons/react';
import { Link, useParams } from '@remix-run/react';
import { useState } from 'react';
import { toast } from '~/components/molecule/toast';
Expand Down Expand Up @@ -58,12 +58,20 @@ const ExtraButton = ({ onDelete }: { onDelete: () => void }) => {

const RepoUrlView = ({ name }: { name: string }) => {
const { account } = useParams();
const { copy } = useClipboard({
onSuccess() {
toast.success('Registry url copied successfully.');
},
});
const { copy } = useClipboard({});
const url = `${registryHost}/${account}/${name}`;
const [copied, setCopied] = useState(false);

const handleCopy = () => {
copy(url);
setCopied(true);
toast.success('Registry url copied successfully.');

setTimeout(() => {
setCopied(false);
}, 3000);
};

Comment on lines +65 to +73
Copy link

Choose a reason for hiding this comment

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

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.

return (
<ListBody
data={
Expand All @@ -72,14 +80,22 @@ const RepoUrlView = ({ name }: { name: string }) => {
onClick={(e) => {
e.preventDefault();
e.stopPropagation();
copy(url);
if (!copied) {
handleCopy();
}
}}
Comment on lines +83 to +85
Copy link

Choose a reason for hiding this comment

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

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.

title={url}
>
<span className="truncate">{url}</span>
<span>
<Copy size={16} />
</span>
{copied ? (
<span>
<Check size={16} />
</span>
) : (
<span>
<Copy size={16} />
</span>
)}
</div>
}
/>
Expand Down
Loading