Draft
Conversation
Add CopyField component for displaying copyable values with visual feedback: - Clipboard API integration via navigator.clipboard.writeText - Tooltip feedback showing 'Copied!' or 'Copy failed' states - Monospace font option for code values - 1.5 second auto-hide timeout for feedback - Full test coverage with 5 test cases - Accessible with aria-label and role=status Also add test setup file with jest-dom matchers and cleanup.
Contributor
⏭️ Storybook build skipped
Storybook build was intentionally skipped for this PR event. Relevant files
|
Contributor
PR Reviewer Guide 🔍(Review updated until commit 540f34d)Here are some key observations to aid the review process:
|
Contributor
|
Persistent review updated to latest commit 540f34d |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
+30
to
+61
| const hideTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| const buttonRef = useRef<HTMLButtonElement>(null); | ||
|
|
||
| const handleCopy = useCallback( | ||
| async (event: React.MouseEvent<HTMLButtonElement>) => { | ||
| // Calculate tooltip position | ||
| const rect = event.currentTarget.getBoundingClientRect(); | ||
| setTooltipPosition({ | ||
| x: rect.left + rect.width / 2, | ||
| y: rect.top - 8, | ||
| }); | ||
|
|
||
| try { | ||
| await navigator.clipboard.writeText(value); | ||
| setCopyStatus('copied'); | ||
| } catch { | ||
| setCopyStatus('failed'); | ||
| } | ||
|
|
||
| // Clear any existing timeout | ||
| if (hideTimeoutRef.current) { | ||
| clearTimeout(hideTimeoutRef.current); | ||
| } | ||
|
|
||
| // Reset status after delay | ||
| hideTimeoutRef.current = setTimeout(() => { | ||
| setCopyStatus('idle'); | ||
| }, COPY_FEEDBACK_DURATION_MS); | ||
| }, | ||
| [value], | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
Suggestion: Add a useEffect cleanup to clear the pending timeout when the component unmounts. This prevents memory leaks and state updates on unmounted components if the user navigates away while the copied feedback tooltip is still visible. [possible issue, importance: 7]
Suggested change
| const hideTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const buttonRef = useRef<HTMLButtonElement>(null); | |
| const handleCopy = useCallback( | |
| async (event: React.MouseEvent<HTMLButtonElement>) => { | |
| // Calculate tooltip position | |
| const rect = event.currentTarget.getBoundingClientRect(); | |
| setTooltipPosition({ | |
| x: rect.left + rect.width / 2, | |
| y: rect.top - 8, | |
| }); | |
| try { | |
| await navigator.clipboard.writeText(value); | |
| setCopyStatus('copied'); | |
| } catch { | |
| setCopyStatus('failed'); | |
| } | |
| // Clear any existing timeout | |
| if (hideTimeoutRef.current) { | |
| clearTimeout(hideTimeoutRef.current); | |
| } | |
| // Reset status after delay | |
| hideTimeoutRef.current = setTimeout(() => { | |
| setCopyStatus('idle'); | |
| }, COPY_FEEDBACK_DURATION_MS); | |
| }, | |
| [value], | |
| ); | |
| const hideTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const buttonRef = useRef<HTMLButtonElement>(null); | |
| // Cleanup timeout on unmount | |
| useEffect(() => { | |
| return () => { | |
| if (hideTimeoutRef.current) { | |
| clearTimeout(hideTimeoutRef.current); | |
| } | |
| }; | |
| }, []); | |
| const handleCopy = useCallback( | |
| async (event: React.MouseEvent<HTMLButtonElement>) => { | |
| // ... existing code ... | |
| hideTimeoutRef.current = setTimeout(() => { | |
| setCopyStatus('idle'); | |
| }, COPY_FEEDBACK_DURATION_MS); | |
| }, | |
| [value], | |
| ); |
Comment on lines
+11
to
+36
| useEffect(() => { | ||
| if (isOpen && icon) { | ||
| // Fetch metadata from Iconify API with collection info | ||
| iconifyApi | ||
| .listIcons({ prefix, info: true }) | ||
| .then(({ icons, info }) => { | ||
| const foundIcon = icons.find((i) => i.name === icon.name); | ||
| if (foundIcon) { | ||
| setMetadata({ | ||
| name: foundIcon.name, | ||
| iconId: `${prefix}:${foundIcon.name}`, | ||
| tags: Array.from(foundIcon.categories || new Set()), | ||
| collection: info?.name ?? prefix, | ||
| license: info?.license?.title ?? 'Unknown License', | ||
| }); | ||
| } else { | ||
| setMetadata(null); | ||
| } | ||
| }) | ||
| .catch(() => { | ||
| setMetadata(null); | ||
| }); | ||
| } else { | ||
| setMetadata(null); | ||
| } | ||
| }, [isOpen, icon, prefix]); |
Contributor
There was a problem hiding this comment.
Suggestion: Prevent state updates on unmounted components by tracking the mounted state or using an AbortController. The async listIcons call may complete after the modal closes or unmounts, causing React warnings or memory leaks. [possible issue, importance: 7]
Suggested change
| useEffect(() => { | |
| if (isOpen && icon) { | |
| // Fetch metadata from Iconify API with collection info | |
| iconifyApi | |
| .listIcons({ prefix, info: true }) | |
| .then(({ icons, info }) => { | |
| const foundIcon = icons.find((i) => i.name === icon.name); | |
| if (foundIcon) { | |
| setMetadata({ | |
| name: foundIcon.name, | |
| iconId: `${prefix}:${foundIcon.name}`, | |
| tags: Array.from(foundIcon.categories || new Set()), | |
| collection: info?.name ?? prefix, | |
| license: info?.license?.title ?? 'Unknown License', | |
| }); | |
| } else { | |
| setMetadata(null); | |
| } | |
| }) | |
| .catch(() => { | |
| setMetadata(null); | |
| }); | |
| } else { | |
| setMetadata(null); | |
| } | |
| }, [isOpen, icon, prefix]); | |
| useEffect(() => { | |
| let isMounted = true; | |
| if (isOpen && icon) { | |
| // Fetch metadata from Iconify API with collection info | |
| iconifyApi | |
| .listIcons({ prefix, info: true }) | |
| .then(({ icons, info }) => { | |
| if (!isMounted) return; | |
| const foundIcon = icons.find((i) => i.name === icon.name); | |
| if (foundIcon) { | |
| setMetadata({ | |
| name: foundIcon.name, | |
| iconId: `${prefix}:${foundIcon.name}`, | |
| tags: Array.from(foundIcon.categories || new Set()), | |
| collection: info?.name ?? prefix, | |
| license: info?.license?.title ?? 'Unknown License', | |
| }); | |
| } else { | |
| setMetadata(null); | |
| } | |
| }) | |
| .catch(() => { | |
| if (isMounted) setMetadata(null); | |
| }); | |
| } else { | |
| setMetadata(null); | |
| } | |
| return () => { | |
| isMounted = false; | |
| }; | |
| }, [isOpen, icon, prefix]); |
Comment on lines
+136
to
+142
| <div style={{ marginBottom: '16px' }}> | ||
| <CopyableText | ||
| value={iconName} | ||
| label="" | ||
| size="lg" | ||
| /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Suggestion: Wrap the icon name in a heading element (e.g., <h2>) to satisfy accessibility requirements for dialog titles and match test expectations. The modal currently lacks a programmatically identifiable heading despite the test comment indicating one should exist. [general, importance: 6]
Suggested change
| <div style={{ marginBottom: '16px' }}> | |
| <CopyableText | |
| value={iconName} | |
| label="" | |
| size="lg" | |
| /> | |
| </div> | |
| <h2 style={{ marginTop: 0, marginBottom: '16px', fontSize: '1.5rem', fontWeight: 600 }}> | |
| {iconName} | |
| </h2> | |
| <div style={{ marginBottom: '16px' }}> | |
| <CopyableText | |
| value={iconName} | |
| label="" | |
| size="lg" | |
| /> | |
| </div> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Checklist
Please check if your PR fulfills the following requirements:
Problem
For each icon in the icon gallery, we would like to let user see details about the icon such as tags and easily copy the name or custom element tag with the right name to use in their projects.
Solution
Add a icon details modal
Demo
Visit the icon gallery page and click on an icon:
https://infomaniak.github.io/design-system/storybook/mr/108/?path=/docs/icons-icon-gallery--docs&collection=esds