refactor: extract ShareDialog in ShareItem component#6801
Closed
thomastayler wants to merge 14 commits into
Closed
refactor: extract ShareDialog in ShareItem component#6801thomastayler wants to merge 14 commits into
thomastayler wants to merge 14 commits into
Conversation
…nd add ImageFocus icon
…ring query and LinkTab component
…Item, streamline ShareModal with new tab structure for sharing options
…friendly button layouts and enhancing accessibility features
…t and accessibility features, remove unused QrCodeDialog and related files
Contributor
## Walkthrough
A new unified `ShareModal` component is introduced to replace the previous dialog-based sharing UI in the journeys-admin app. This change removes separate dialog components for editing URLs, embedding, and QR codes, consolidating their functionality into tabbed subcomponents within the modal. Associated tests, stories, and exports for the old dialogs are deleted or refactored accordingly.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------|
| .../ShareModal/ShareModal.tsx, .../ShareModal/tabs/LinkTab/LinkTab.tsx, .../EmbedTab/EmbedTab.tsx, ... | Added new `ShareModal` component and tab subcomponents (`LinkTab`, `EmbedTab`, `QrCodeTab`, `EmbedCardPreview`) for unified sharing. |
| .../ShareModal/ShareModal.spec.tsx | Added test suite for `ShareModal` covering modal visibility, tab behavior, and user interactions. |
| .../ShareModal/index.ts, .../tabs/LinkTab/index.ts, .../tabs/EmbedTab/index.ts, .../tabs/QrCodeTab/index.ts | Added re-exports for new sharing components and tabs. |
| .../Toolbar/Items/ShareItem/ShareItem.tsx, .../ShareItem.spec.tsx | Refactored `ShareItem` to use `ShareModal`; updated and simplified tests for new sharing flow. |
| .../Toolbar/Items/ShareItem/SlugDialog/*, .../EmbedJourneyDialog/*, .../QrCodeDialog/* | Deleted old dialog components, their stories, tests, and index exports. |
| .../ShareModal/tabs/QrCodeTab/QrCodeTab.tsx, .../ScanCount/ScanCount.tsx | Refactored QR code dialog into tab panel; updated layout and structure, minor logging added. |
| .../useJourneyForShareLazyQuery/useJourneyForShareLazyQuery.ts | Extended sharing query to include journey title, description, and primary image block details. |
| .../Tooltip/Tooltip.tsx | Added `light` and `leaveTouchDelay` props for tooltip styling and behavior. |
| .../icons/Icon.tsx, .../icons/ImageFocus.tsx, .../icons/icon.stories.tsx | Added new `ImageFocus` icon to the icon system and stories. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant ShareItem
participant ShareModal
participant LinkTab
participant QrCodeTab
participant EmbedTab
User->>ShareItem: Click Share button
ShareItem->>ShareModal: Open modal (open=true, journey, hostname)
ShareModal->>User: Show tabbed UI (Link, QR, Embed)
User->>ShareModal: Switch tab (Link/QR/Embed)
ShareModal->>LinkTab: Render LinkTab (if selected)
ShareModal->>QrCodeTab: Render QrCodeTab (if selected)
ShareModal->>EmbedTab: Render EmbedTab (if selected)
User->>ShareModal: Click Close
ShareModal->>ShareItem: onClose()Possibly related PRs
Suggested reviewers
|
Contributor
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
Contributor
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.spec.tsx (1)
95-95: Replace delete operator with undefined assignment for better performance.The delete operator can impact performance and has side effects on the original object.
- delete process.env.NEXT_PUBLIC_JOURNEYS_URL + process.env.NEXT_PUBLIC_JOURNEYS_URL = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 95-95: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/QrCodeTab.tsx (1)
144-144: Consider making TabPanel value configurable.The hardcoded value "1" makes the component less flexible and could cause issues if tab ordering changes. Consider accepting the tab value as a prop or using a more descriptive constant.
-interface QrCodeTabProps { +interface QrCodeTabProps { + value?: string journey?: JourneyFromContext | JourneyFromLazyQuery } -export function QrCodeTab({ journey }: QrCodeTabProps): ReactElement { +export function QrCodeTab({ value = "qr-code", journey }: QrCodeTabProps): ReactElement { - <TabPanel value="1" sx={{ padding: 0 }}> + <TabPanel value={value} sx={{ padding: 0 }}>apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedTab.tsx (1)
60-60: Consider making TabPanel value configurable.Similar to QrCodeTab, the hardcoded value "2" should be made configurable for better maintainability and flexibility.
interface EmbedTabProps { + value?: string journey?: JourneyFromContext | JourneyFromLazyQuery } -export function EmbedTab({ journey }: EmbedTabProps): ReactElement { +export function EmbedTab({ value = "embed", journey }: EmbedTabProps): ReactElement { - <TabPanel value="2" sx={{ padding: 0, pb: 3 }}> + <TabPanel value={value} sx={{ padding: 0, pb: 3 }}>apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedCardPreview/EmbedCardPreview.tsx (1)
18-18: Remove unnecessary styled component.The
StyledIframecomponent is defined but provides no styling. You can use a regulariframeelement directly.-const StyledIframe = styled('iframe')(() => ({})) - <StyledIframe + <iframe src={iframeSrc} loading="lazy" onLoad={handleIframeLoad} onError={handleIframeError}apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/LinkTab.tsx (5)
64-64: Remove unnecessary await on setValuesThe
setValuesfunction from Formik is synchronous, so theawaitis not needed here.- await setValues({ slug: response?.data?.journeyUpdate.slug }) + setValues({ slug: response?.data?.journeyUpdate.slug })
145-148: Remove redundant empty string fallbackThe
srcprop is already checked for null in the conditional, so the fallback to empty string is unnecessary.{journey?.primaryImageBlock?.src != null ? ( <Image - src={journey?.primaryImageBlock?.src ?? ''} + src={journey.primaryImageBlock.src} alt="Journey Image"
307-314: Remove empty Box elementThis Box element is empty and serves no purpose. Consider removing it to keep the code clean.
- <Box - sx={{ - display: 'flex', - alignItems: 'center', - justifyContent: 'flex-end' - }} - ></Box>
318-424: Consider extracting helper components for better performanceThe helper components are recreated on each render. While not critical given their size, you could improve performance by moving them outside the main component or memoizing them with
React.memo.If you decide to refactor, you would need to pass the required dependencies as props instead of relying on closure variables.
147-148: Improve image alt text for better accessibilityConsider using a more descriptive alt text that includes the journey title for better accessibility.
src={journey.primaryImageBlock.src} - alt="Journey Image" + alt={journey?.title ? `${journey.title} cover image` : 'Journey cover image'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/journeys-admin/__generated__/GetJourneyForSharing.tsis excluded by!**/__generated__/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.spec.tsx(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.tsx(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/index.ts(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedCardPreview/EmbedCardPreview.tsx(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedTab.tsx(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/index.ts(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/LinkTab.tsx(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/index.ts(1 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/QrCodeTab.tsx(3 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/ScanCount/ScanCount.tsx(3 hunks)apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/index.ts(1 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedCardPreview/EmbedCardPreview.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.spec.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.stories.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/index.ts(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/QrCodeDialog.spec.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/QrCodeDialog.stories.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/index.ts(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.spec.tsx(10 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx(5 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.spec.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.stories.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.tsx(0 hunks)apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/index.ts(0 hunks)apps/journeys-admin/src/components/Tooltip/Tooltip.tsx(2 hunks)apps/journeys-admin/src/libs/useJourneyForShareLazyQuery/useJourneyForShareLazyQuery.ts(2 hunks)libs/shared/ui/src/components/icons/Icon.tsx(3 hunks)libs/shared/ui/src/components/icons/ImageFocus.tsx(1 hunks)libs/shared/ui/src/components/icons/icon.stories.tsx(1 hunks)
💤 Files with no reviewable changes (12)
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/index.ts
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/index.ts
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/index.ts
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.stories.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.stories.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedJourneyDialog.spec.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.spec.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/QrCodeDialog.stories.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/SlugDialog/SlugDialog.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/QrCodeDialog/QrCodeDialog.spec.tsx
- apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/EmbedJourneyDialog/EmbedCardPreview/EmbedCardPreview.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedTab.tsx (2)
apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/index.ts (1)
EmbedTab(1-1)apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedCardPreview/EmbedCardPreview.tsx (1)
EmbedCardPreview(24-206)
apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/LinkTab.tsx (4)
apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/index.ts (1)
LinkTab(1-1)apis/api-journeys/src/app/modules/journey/journey.resolver.ts (1)
journeyUpdate(735-838)libs/journeys/ui/src/components/TextField/TextField.tsx (1)
TextField(41-76)apps/journeys-admin/src/components/Tooltip/Tooltip.tsx (1)
Tooltip(46-95)
🪛 Biome (1.9.4)
apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.spec.tsx
[error] 95-95: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Deploy Preview and Test (watch, 6801/merge, pull_request, 22)
- GitHub Check: Deploy Preview and Test (journeys-admin, 6801/merge, pull_request, 22)
- GitHub Check: test (22, 3/3)
- GitHub Check: test (22, 1/3)
- GitHub Check: test (22, 2/3)
- GitHub Check: build (22)
🔇 Additional comments (31)
libs/shared/ui/src/components/icons/icon.stories.tsx (1)
114-114: Add new 'ImageFocus' icon to storybook.
The new icon name is now included in the Storybook preview list, matching the updated icon set.apps/journeys-admin/src/components/Editor/ShareModal/index.ts (1)
1-1: Re-exportShareModalfor centralized import.
This barrel file now exposes the new ShareModal component, aligning with the consolidated sharing UI.libs/shared/ui/src/components/icons/Icon.tsx (3)
106-106: Import newImageFocusicon component.
Placed correctly between Image3 and ImageX in alphabetical order.
321-321: UpdateIconNameunion with 'ImageFocus'.
Ensures the type system recognizes the new icon.
540-540: IncludeImageFocusiniconComponentsmapping.
Maps the union entry to the imported React component.apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/index.ts (1)
1-1: Re-exportQrCodeTabfor tab integration.
Exposes the QR code sharing tab for use in the unified ShareModal.apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/index.ts (1)
1-1: Re-exportEmbedTabfor tab integration.
Makes the embed-sharing tab available to the ShareModal.apps/journeys-admin/src/components/Editor/ShareModal/tabs/LinkTab/index.ts (1)
1-1: LGTM!Clean barrel export following standard patterns.
libs/shared/ui/src/components/icons/ImageFocus.tsx (1)
3-10: LGTM!Well-structured SVG icon component following Material-UI patterns. The SVG path data looks valid and the component is properly exported.
apps/journeys-admin/src/libs/useJourneyForShareLazyQuery/useJourneyForShareLazyQuery.ts (1)
13-14: Good enhancement for sharing functionality.Adding
titleanddescriptionfields will improve the sharing experience by providing rich metadata.apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.tsx (5)
4-4: LGTM: Clean consolidation of imports.The refactoring successfully consolidates multiple dialog imports into a single
ShareModalimport, simplifying the component's dependencies.Also applies to: 12-12
46-46: Well-designed state consolidation.The single
showShareModalstate variable effectively replaces the need for multiple dialog state variables, significantly simplifying state management.
48-52: Simplified event handler looks good.The
handleShowMenufunction is now cleaner without needing to manage anchor elements for multiple dialogs.
63-65: Good encapsulation of route logic.The new
handleRouteChangefunction properly encapsulates the route management logic, making it reusable if needed.
76-84:Details
❓ Verification inconclusive
Verify that ShareModal handles all previous dialog functionality.
The ShareModal integration looks clean, but please ensure that all functionality from the previous separate dialogs (
SlugDialog,EmbedJourneyDialog,QrCodeDialog) is preserved in the new tabbed interface.
🏁 Script executed:
#!/bin/bash # Description: Verify that removed dialog components are no longer referenced in the codebase # Expected: No remaining imports or usages of the old dialog components # Search for any remaining references to the old dialog components echo "Checking for remaining references to old dialog components..." echo -e "\n=== SlugDialog references ===" rg -l "SlugDialog" --type tsx --type ts || echo "No SlugDialog references found" echo -e "\n=== EmbedJourneyDialog references ===" rg -l "EmbedJourneyDialog" --type tsx --type ts || echo "No EmbedJourneyDialog references found" echo -e "\n=== QrCodeDialog references ===" rg -l "QrCodeDialog" --type tsx --type ts || echo "No QrCodeDialog references found" echo -e "\n=== Verification complete ==="Length of output: 899
#!/bin/bash # Locate the ShareModal component file echo "Locating ShareModal.tsx..." FILE=$(rg --files --glob '*ShareModal.tsx' | head -n1) if [[ -z "$FILE" ]]; then echo "ShareModal.tsx not found" exit 1 fi echo "Found file: $FILE" # Search for tab implementation and each functionality echo -e "\n=== 'Tab' occurrences in ShareModal ===" rg -n "Tab" "$FILE" || echo "No 'Tab' patterns found" echo -e "\n=== 'slug' or 'Slug' in ShareModal ===" rg -n "slug" "$FILE" || echo "No lowercase 'slug' found" rg -n "Slug" "$FILE" || echo "No uppercase 'Slug' found" echo -e "\n=== 'embed' or 'Embed' in ShareModal ===" rg -n "embed" "$FILE" || echo "No lowercase 'embed' found" rg -n "Embed" "$FILE" || echo "No uppercase 'Embed' found" echo -e "\n=== 'qr' or 'Qr' in ShareModal ===" rg -n "qr" "$FILE" || echo "No lowercase 'qr' found" rg -n "QR" "$FILE" || echo "No uppercase 'QR' found" echo -e "\n=== 'QRCode' in ShareModal ===" rg -n "QRCode" "$FILE" || echo "No 'QRCode' found" echo -e "\n=== Verification complete ==="
#!/bin/bash # Locate the ShareModal component file echo "Locating ShareModal component file..." FILE=$(rg --files --glob '*ShareModal*.tsx' | head -n1) if [[ -z "$FILE" ]]; then FILE=$(find . -type f -iname '*ShareModal*.tsx' | head -n1) fi if [[ -z "$FILE" ]]; then echo "Error: ShareModal component file not found" exit 1 fi echo "Found: $FILE" # Check for tab implementation (common Tab, Tabs, TabList, TabPanel patterns) echo -e "\n=== Tab-related patterns ===" rg -nE 'Tab(List|Panel)?s?\b' "$FILE" || echo "No tab patterns found" # Verify presence of slug functionality echo -e "\n=== slug / Slug ===" rg -nE '\bslug\b|\bSlug\b' "$FILE" || echo "No slug references found" # Verify presence of embed functionality echo -e "\n=== embed / Embed ===" rg -nE '\bembed\b|\bEmbed\b' "$FILE" || echo "No embed references found" # Verify presence of QR code functionality echo -e "\n=== qr / QR / QRCode ===" rg -nE '\bqr\b|\bQR\b|\bQRCode\b' "$FILE" || echo "No QR code references found" echo -e "\nVerification complete."
Verify ShareModal preserves slug, embed & QR functionality
Please ensure the new tabbedShareModal(lines 76–84) fully replaces the previousSlugDialog,EmbedJourneyDialog, andQrCodeDialogfeatures:
- Confirm there are three tabs (Slug, Embed, QR) and each renders the correct UI.
- Validate each tab’s output matches the old dialogs (exporting a slug, generating embed code, producing a QR code).
- Remove any leftover imports or code referencing the old dialogs.
apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.spec.tsx (1)
1-173: Excellent test coverage for the new ShareModal component.The test suite comprehensively covers all key functionality including:
- Modal rendering and visibility states
- Loading behavior with null journey data
- URL construction with different domain configurations
- User interaction callbacks
- Clipboard copy functionality
This provides good confidence that the ShareModal properly replaces the previous dialog components.
🧰 Tools
🪛 Biome (1.9.4)
[error] 95-95: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
apps/journeys-admin/src/components/Tooltip/Tooltip.tsx (2)
42-43: Well-designed prop additions for enhanced customization.The new
lightandleaveTouchDelayprops provide valuable customization options while maintaining backward compatibility through optional typing and sensible defaults.
64-78: Clean implementation of light theme styling.The conditional light theme styling provides a polished alternative appearance with proper attention to:
- Color contrast (white background with primary text)
- Visual hierarchy (border and shadow effects)
- Consistency (matching arrow styling with shadow)
This enhancement will be valuable for the new ShareModal components that likely need varied tooltip styles.
apps/journeys-admin/src/components/Editor/ShareModal/ShareModal.tsx (5)
28-33: Well-designed component interface.The props interface is clean and properly typed, providing exactly what's needed for the sharing functionality without unnecessary complexity.
54-59: Good responsive design pattern implementation.The use of
useMediaQueryto switch between Dialog and SwipeableDrawer provides an excellent responsive user experience that follows Material-UI best practices.
63-71: Proper loading state handling.The loading spinner provides good user feedback when journey data is not yet available, preventing confusion and maintaining a professional experience.
74-84: Excellent tab organization and component separation.The tabbed interface cleanly organizes the three sharing options (Link, QR Code, Embed) with appropriate icons and delegates functionality to dedicated tab components, promoting good separation of concerns.
108-153: Well-implemented mobile drawer with proper accessibility.The SwipeableDrawer implementation includes:
- Required
onOpenhandler for accessibility compliance- Proper styling with rounded corners and responsive padding
- Clean header with title and close button
- Good minimum height for usability
The mobile experience will be intuitive and accessible.
apps/journeys-admin/src/components/Editor/ShareModal/tabs/QrCodeTab/QrCodeTab.tsx (1)
162-190: LGTM! Excellent responsive layout implementation.The responsive layout design is well-implemented with proper breakpoints, flexible spacing, and appropriate alignment adjustments for different screen sizes. The component structure is clean and maintainable.
apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedTab.tsx (2)
63-103: LGTM! Excellent responsive design with mobile-first approach.The responsive implementation using conditional rendering for desktop vs mobile (accordion) is well-executed. The accordion provides a clean way to hide the preview on smaller screens while maintaining accessibility.
155-166: Good security practices with external link.The external link to terms of agreement properly includes
rel="noopener"andtarget="_blank"for security and user experience.apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedCardPreview/EmbedCardPreview.tsx (2)
36-39: LGTM! Good use of memoization.Properly memoizing the iframe src prevents unnecessary re-renders and improves performance. The dependency array is correct.
123-183: LGTM! Excellent loading and error state handling.The component properly manages loading and error states with appropriate visual feedback. The opacity transition for the iframe provides a smooth user experience.
apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.spec.tsx (3)
58-88: LGTM! Enhanced mock data aligns with GraphQL changes.The addition of
title,description, andprimaryImageBlockto the journey mock properly reflects the expanded GraphQL query fields mentioned in the AI summary.
142-167: LGTM! Clear test structure for modal opening behavior.The test properly verifies that the ShareModal opens when the Share button is clicked, with appropriate assertions for dialog presence.
169-293: LGTM! Navigation tests properly verify router integration.The tests for edit URL, embed journey, and QR code navigation properly verify that the correct router.push calls are made with the expected query parameters.
Comment on lines
+51
to
+57
| const handleCopyCode = async (): Promise<void> => { | ||
| await navigator.clipboard.writeText(iframeLink ?? '') | ||
| enqueueSnackbar(t('Code Copied'), { | ||
| variant: 'success', | ||
| preventDuplicate: true | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for clipboard operations.
The clipboard write operation should include error handling in case the operation fails (e.g., in insecure contexts or when permissions are denied).
const handleCopyCode = async (): Promise<void> => {
- await navigator.clipboard.writeText(iframeLink ?? '')
- enqueueSnackbar(t('Code Copied'), {
- variant: 'success',
- preventDuplicate: true
- })
+ try {
+ await navigator.clipboard.writeText(iframeLink ?? '')
+ enqueueSnackbar(t('Code Copied'), {
+ variant: 'success',
+ preventDuplicate: true
+ })
+ } catch (error) {
+ enqueueSnackbar(t('Failed to copy code'), {
+ variant: 'error',
+ preventDuplicate: true
+ })
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleCopyCode = async (): Promise<void> => { | |
| await navigator.clipboard.writeText(iframeLink ?? '') | |
| enqueueSnackbar(t('Code Copied'), { | |
| variant: 'success', | |
| preventDuplicate: true | |
| }) | |
| } | |
| const handleCopyCode = async (): Promise<void> => { | |
| try { | |
| await navigator.clipboard.writeText(iframeLink ?? '') | |
| enqueueSnackbar(t('Code Copied'), { | |
| variant: 'success', | |
| preventDuplicate: true | |
| }) | |
| } catch (error) { | |
| enqueueSnackbar(t('Failed to copy code'), { | |
| variant: 'error', | |
| preventDuplicate: true | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
apps/journeys-admin/src/components/Editor/ShareModal/tabs/EmbedTab/EmbedTab.tsx
between lines 51 and 57, the clipboard write operation lacks error handling.
Wrap the navigator.clipboard.writeText call in a try-catch block to catch any
errors during the copy operation. In the catch block, use enqueueSnackbar to
display an error message indicating the copy failed, ensuring the user is
informed if the clipboard operation does not succeed.
Comment on lines
+375
to
398
| mockedUseRouter.mockReturnValue({ | ||
| query: { param: null }, | ||
| push, | ||
| events: { on } | ||
| } as unknown as NextRouter) | ||
|
|
||
| render( | ||
| <SnackbarProvider> | ||
| <MockedProvider mocks={[journeyForSharingMock]}> | ||
| <JourneyProvider | ||
| value={{ journey: defaultJourney, variant: 'admin' }} | ||
| > | ||
| <ShareItem variant="button" journey={defaultJourney} /> | ||
| </JourneyProvider> | ||
| </MockedProvider> | ||
| </SnackbarProvider> | ||
| ) | ||
|
|
||
| fireEvent.click(screen.getByRole('button', { name: 'Share' })) | ||
| expect(screen.getByRole('dialog')).toBeInTheDocument() | ||
|
|
||
| expect(screen.getByRole('dialog')).toBeInTheDocument() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complete the test implementation for closing dialog.
The test case for closing the ShareDialog when clicking outside is incomplete. It opens the dialog but doesn't actually test the closing behavior.
it('should close ShareDialog when clicking outside', () => {
mockedUseRouter.mockReturnValue({
query: { param: null },
push,
events: { on }
} as unknown as NextRouter)
render(
<SnackbarProvider>
<MockedProvider mocks={[journeyForSharingMock]}>
<JourneyProvider
value={{ journey: defaultJourney, variant: 'admin' }}
>
<ShareItem variant="button" journey={defaultJourney} />
</JourneyProvider>
</MockedProvider>
</SnackbarProvider>
)
fireEvent.click(screen.getByRole('button', { name: 'Share' }))
expect(screen.getByRole('dialog')).toBeInTheDocument()
- expect(screen.getByRole('dialog')).toBeInTheDocument()
+ // Click outside the dialog (on the backdrop)
+ fireEvent.click(document.body)
+
+ expect(screen.queryByRole('dialog')).not.toBeInTheDocument()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should close ShareDialog when clicking outside', () => { | |
| mockedUseRouter.mockReturnValue({ | |
| query: { param: null }, | |
| push, | |
| events: { on } | |
| } as unknown as NextRouter) | |
| render( | |
| <SnackbarProvider> | |
| <MockedProvider mocks={[journeyForSharingMock]}> | |
| <JourneyProvider | |
| value={{ journey: defaultJourney, variant: 'admin' }} | |
| > | |
| <ShareItem variant="button" journey={defaultJourney} /> | |
| </JourneyProvider> | |
| </MockedProvider> | |
| </SnackbarProvider> | |
| ) | |
| fireEvent.click(screen.getByRole('button', { name: 'Share' })) | |
| expect(screen.getByRole('dialog')).toBeInTheDocument() | |
| expect(screen.getByRole('dialog')).toBeInTheDocument() | |
| }) | |
| it('should close ShareDialog when clicking outside', () => { | |
| mockedUseRouter.mockReturnValue({ | |
| query: { param: null }, | |
| push, | |
| events: { on } | |
| } as unknown as NextRouter) | |
| render( | |
| <SnackbarProvider> | |
| <MockedProvider mocks={[journeyForSharingMock]}> | |
| <JourneyProvider | |
| value={{ journey: defaultJourney, variant: 'admin' }} | |
| > | |
| <ShareItem variant="button" journey={defaultJourney} /> | |
| </JourneyProvider> | |
| </MockedProvider> | |
| </SnackbarProvider> | |
| ) | |
| fireEvent.click(screen.getByRole('button', { name: 'Share' })) | |
| expect(screen.getByRole('dialog')).toBeInTheDocument() | |
| // Click outside the dialog (on the backdrop) | |
| fireEvent.click(document.body) | |
| expect(screen.queryByRole('dialog')).not.toBeInTheDocument() | |
| }) |
🤖 Prompt for AI Agents
In
apps/journeys-admin/src/components/Editor/Toolbar/Items/ShareItem/ShareItem.spec.tsx
around lines 375 to 398, the test opens the ShareDialog but does not verify that
it closes when clicking outside. To fix this, simulate a click event outside the
dialog after it opens and then assert that the dialog is no longer present in
the document, confirming it has closed properly.
… clean up code and improve readability
…alt text and blur placeholder
Contributor
|
This pull request has been marked stale due to {daysBeforePrStale} days without activity. It will be closed in {daysBeforePrClose} days unless updated. |
Contributor
|
This pull request has been marked stale due to 28 days without activity. It will be closed in 14 days unless updated. |
Contributor
|
This pull request was automatically closed after remaining stale for 14 days. |
github-actions
Bot
deleted the
tommytayler/prj-1569-refactor-share-dialog
branch
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.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores