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

Added image upload handling to CTA Card #1421

Merged
merged 5 commits into from
Jan 27, 2025
Merged

Added image upload handling to CTA Card #1421

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58a0371
Added image uploading to CTA Card
ronaldlangeveld Jan 27, 2025
35722e7
Updated CTA Card tests
ronaldlangeveld Jan 27, 2025
d835bd7
Cleanup comments
ronaldlangeveld Jan 27, 2025
4a05742
Merge branch 'main' into plg-323
ronaldlangeveld Jan 27, 2025
5c18f35
Added preview image test
ronaldlangeveld Jan 27, 2025
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
18 changes: 15 additions & 3 deletions packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,10 @@ export function CtaCard({
updateHasSponsorLabel,
updateLayout,
handleColorChange,
handleButtonColor
handleButtonColor,
onFileChange,
setFileInputRef,
onRemoveMedia
}) {
const [buttonColorPickerExpanded, setButtonColorPickerExpanded] = useState(false);

Expand Down Expand Up @@ -137,8 +140,11 @@ export function CtaCard({
icon='file'
label='Image'
mimeTypes={['image/*']}
setFileInputRef={setFileInputRef}
size='xsmall'
src={imageSrc}
onFileChange={onFileChange}
onRemoveMedia={onRemoveMedia}
/>
{/* Button settings */}
<ToggleSetting
Expand Down Expand Up @@ -328,7 +334,10 @@ CtaCard.propTypes = {
updateShowButton: PropTypes.func,
updateLayout: PropTypes.func,
handleColorChange: PropTypes.func,
handleButtonColor: PropTypes.func
handleButtonColor: PropTypes.func,
onFileChange: PropTypes.func,
setFileInputRef: PropTypes.func,
onRemoveMedia: PropTypes.func
};

CtaCard.defaultProps = {
Expand All @@ -347,5 +356,8 @@ CtaCard.defaultProps = {
updateShowButton: () => {},
updateLayout: () => {},
handleColorChange: () => {},
handleButtonColor: () => {}
handleButtonColor: () => {},
onFileChange: () => {},
setFileInputRef: () => {},
onRemoveMedia: () => {}
};
34 changes: 32 additions & 2 deletions packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import CardContext from '../context/CardContext';
import KoenigComposerContext from '../context/KoenigComposerContext.jsx';
import React from 'react';
import React, {useRef} from 'react';
import {$getNodeByKey} from 'lexical';
import {ActionToolbar} from '../components/ui/ActionToolbar.jsx';
import {CtaCard} from '../components/ui/cards/CtaCard';
Expand All @@ -26,14 +26,18 @@ export const CallToActionNodeComponent = ({
}) => {
const [editor] = useLexicalComposerContext();
const {isEditing, isSelected, setEditing} = React.useContext(CardContext);
const {cardConfig} = React.useContext(KoenigComposerContext);
const {fileUploader, cardConfig} = React.useContext(KoenigComposerContext);
const [showSnippetToolbar, setShowSnippetToolbar] = React.useState(false);
const handleToolbarEdit = (event) => {
event.preventDefault();
event.stopPropagation();
setEditing(true);
};

const fileInputRef = useRef(null);

const imageUploader = fileUploader.useFileUpload('image');

const toggleShowButton = (event) => {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
Expand Down Expand Up @@ -76,6 +80,28 @@ export const CallToActionNodeComponent = ({
});
};

const handleImageChange = async (files) => {
const result = await imageUploader.upload(files);
// reset original src so it can be replaced with preview and upload progress
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.imageUrl = result?.[0].url;
node.hasImage = true;
});
};
Comment on lines +84 to +92
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling and loading state management.

The image upload implementation should include:

  1. Error handling for failed uploads
  2. Loading state management for better UX
  3. File validation before upload

Consider this implementation:

 const handleImageChange = async (files) => {
+    try {
+        editor.update(() => {
+            const node = $getNodeByKey(nodeKey);
+            node.isLoading = true;
+        });
+
         const result = await imageUploader.upload(files);
-        // reset original src so it can be replaced with preview and upload progress
         editor.update(() => {
             const node = $getNodeByKey(nodeKey);
             node.imageUrl = result?.[0].url;
             node.hasImage = true;
+            node.isLoading = false;
         });
+    } catch (error) {
+        editor.update(() => {
+            const node = $getNodeByKey(nodeKey);
+            node.isLoading = false;
+            node.uploadError = error.message;
+        });
+    }
 };
📝 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 handleImageChange = async (files) => {
const result = await imageUploader.upload(files);
// reset original src so it can be replaced with preview and upload progress
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.imageUrl = result?.[0].url;
node.hasImage = true;
});
};
const handleImageChange = async (files) => {
try {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.isLoading = true;
});
const result = await imageUploader.upload(files);
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.imageUrl = result?.[0].url;
node.hasImage = true;
node.isLoading = false;
});
} catch (error) {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.isLoading = false;
node.uploadError = error.message;
});
}
};


const onFileChange = async (e) => {
handleImageChange(e.target.files);
};
Comment on lines +94 to +96
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add file validation before upload.

The file input handler should validate the files before uploading.

 const onFileChange = async (e) => {
+    const files = e.target.files;
+    if (!files || files.length === 0) {
+        return;
+    }
+
+    const file = files[0];
+    const validTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'];
+    if (!validTypes.includes(file.type)) {
+        editor.update(() => {
+            const node = $getNodeByKey(nodeKey);
+            node.uploadError = 'Invalid file type. Please upload an image.';
+        });
+        return;
+    }
+
     handleImageChange(e.target.files);
 };
📝 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 onFileChange = async (e) => {
handleImageChange(e.target.files);
};
const onFileChange = async (e) => {
const files = e.target.files;
if (!files || files.length === 0) {
return;
}
const file = files[0];
const validTypes = ['image/jpeg', 'image/png', 'image/gif', 'image/webp'];
if (!validTypes.includes(file.type)) {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.uploadError = 'Invalid file type. Please upload an image.';
});
return;
}
handleImageChange(e.target.files);
};


const onRemoveMedia = () => {
editor.update(() => {
const node = $getNodeByKey(nodeKey);
node.imageUrl = '';
node.hasImage = false;
});
};

return (
<>
<CtaCard
Expand All @@ -91,17 +117,21 @@ export const CallToActionNodeComponent = ({
hasSponsorLabel={hasSponsorLabel}
htmlEditor={htmlEditor}
imageSrc={imageUrl}
imageUploader={imageUploader}
isEditing={isEditing}
isSelected={isSelected}
layout={layout}
setEditing={setEditing}
setFileInputRef={ref => fileInputRef.current = ref}
showButton={showButton}
text={textValue}
updateButtonText={handleButtonTextChange}
updateButtonUrl={handleButtonUrlChange}
updateHasSponsorLabel={handleHasSponsorLabelChange}
updateLayout={() => {}}
updateShowButton={toggleShowButton}
onFileChange={onFileChange}
onRemoveMedia={onRemoveMedia}
/>
<ActionToolbar
data-kg-card-toolbar="button"
Expand Down
24 changes: 24 additions & 0 deletions packages/koenig-lexical/test/e2e/cards/cta-card.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import path from 'path';
import {assertHTML, focusEditor, html, initialize, insertCard} from '../../utils/e2e';
import {expect, test} from '@playwright/test';
import {fileURLToPath} from 'url';
const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);

test.describe('Call To Action Card', async () => {
let page;
Expand Down Expand Up @@ -205,4 +209,24 @@ test.describe('Call To Action Card', async () => {
await expect(page.locator(firstChildSelector)).toHaveClass(new RegExp(color.expectedClass));
}
});

test('can add and remove CTA Card image', async function () {
const filePath = path.relative(process.cwd(), __dirname + `/../fixtures/large-image.jpeg`);

await focusEditor(page);
await insertCard(page, {cardName: 'call-to-action'});

const fileChooserPromise = page.waitForEvent('filechooser');

await page.click('[data-testid="media-upload-placeholder"]');

const fileChooser = await fileChooserPromise;
await fileChooser.setFiles([filePath]);

const imgLocator = page.locator('[data-kg-card="call-to-action"] img[src^="blob:"]');
const imgElement = await imgLocator.first();
await expect(imgElement).toHaveAttribute('src', /blob:/);
await page.click('[data-testid="media-upload-remove"]');
await expect(imgLocator).not.toBeVisible();
});
Comment on lines +213 to +231
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add test cases for error scenarios.

The image upload tests should cover error scenarios such as:

  • Invalid file types
  • Upload failures
  • Network errors

Here's an example test case to add:

test('shows error for invalid file type', async function () {
    const filePath = path.relative(process.cwd(), __dirname + '/../fixtures/invalid.txt');

    await focusEditor(page);
    await insertCard(page, {cardName: 'call-to-action'});

    const fileChooserPromise = page.waitForEvent('filechooser');
    await page.click('[data-testid="media-upload-placeholder"]');
    const fileChooser = await fileChooserPromise;
    await fileChooser.setFiles([filePath]);

    await expect(page.locator('[data-testid="upload-error"]')).toBeVisible();
    await expect(page.locator('[data-testid="upload-error"]')).toHaveText('Invalid file type. Please upload an image.');
});

});
Loading