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.

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

Updated CTA card cardwrapper behavior #1414

Merged
merged 11 commits into from
Jan 21, 2025
Prev Previous commit
Next Next commit
Replaced hasImage toggle with image upload setting in CTA card
sanne-san committed Jan 21, 2025
commit a49ed13a0867932b64d3f884f2d4432ac4af14d4
6 changes: 3 additions & 3 deletions packages/koenig-lexical/src/components/ui/SettingsPanel.jsx
Original file line number Diff line number Diff line change
@@ -286,15 +286,15 @@ export function ColorPickerSetting({label, isExpanded, onSwatchChange, onPickerC
);
}

export function MediaUploadSetting({className, label, hideLabel, onFileChange, isDraggedOver, placeholderRef, src, alt, isLoading, errors = [], progress, onRemoveMedia, icon, desc = '', size, borderStyle, mimeTypes, isPinturaEnabled, openImageEditor, setFileInputRef}) {
export function MediaUploadSetting({className, label, hideLabel, onFileChange, isDraggedOver, placeholderRef, src, alt, isLoading, errors = [], progress, onRemoveMedia, icon, desc = '', size, stacked, borderStyle, mimeTypes, isPinturaEnabled, openImageEditor, setFileInputRef}) {
return (
<div className={clsx(className)} data-testid="media-upload-setting">
<div className={clsx(className, !stacked && 'flex justify-between')} data-testid="media-upload-setting">
<div className={hideLabel ? 'sr-only' : 'mb-2 text-sm font-medium tracking-normal text-grey-900 dark:text-grey-400'}>{label}</div>

<MediaUploader
alt={alt}
borderStyle={borderStyle}
className="h-32"
className={clsx(stacked ? 'h-32' : src ? 'h-[5.2rem]' : 'h-[5.2rem] w-[7.2rem]')}
desc={desc}
dragHandler={{isDraggedOver, setRef: placeholderRef}}
errors={errors}
41 changes: 21 additions & 20 deletions packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ import PropTypes from 'prop-types';
import React from 'react';
import ReplacementStringsPlugin from '../../../plugins/ReplacementStringsPlugin';
import {Button} from '../Button';
import {ButtonGroupSetting, ColorOptionSetting, InputSetting, InputUrlSetting, SettingsPanel, ToggleSetting} from '../SettingsPanel';
import {ButtonGroupSetting, ColorOptionSetting, InputSetting, InputUrlSetting, MediaUploadSetting, SettingsPanel, ToggleSetting} from '../SettingsPanel';
import {ReadOnlyOverlay} from '../ReadOnlyOverlay';

export const CALLOUT_COLORS = {
@@ -62,10 +62,10 @@ export function CtaCard({
buttonText,
buttonUrl,
color,
hasImage,
hasSponsorLabel,
htmlEditor,
htmlEditorInitialState,
imageSrc,
isEditing,
isSelected,
layout,
@@ -74,7 +74,6 @@ export function CtaCard({
updateButtonUrl,
updateShowButton,
updateHasSponsorLabel,
updateHasImage,
updateLayout,
handleColorChange
}) {
@@ -100,13 +99,6 @@ export function CtaCard({

const designSettings = (
<>
{/* Layout settings */}
<ButtonGroupSetting
buttons={layoutOptions}
label='Layout'
selectedName={layout}
onClick={updateLayout}
/>
{/* Color picker */}
<ColorOptionSetting
buttons={calloutColorPicker}
@@ -115,17 +107,28 @@ export function CtaCard({
selectedName={color}
onClick={handleColorChange}
/>
{/* Layout settings */}
<ButtonGroupSetting
buttons={layoutOptions}
label='Layout'
selectedName={layout}
onClick={updateLayout}
/>
{/* Sponsor label setting */}
<ToggleSetting
isChecked={hasSponsorLabel}
label='Sponsor label'
onChange={updateHasSponsorLabel}
/>
{/* Image setting */}
<ToggleSetting
isChecked={hasImage}
<MediaUploadSetting
alt='Image'
borderStyle={'rounded'}
icon='file'
label='Image'
onChange={updateHasImage}
mimeTypes={['image/*']}
size='xsmall'
src={imageSrc}
/>
{/* Button settings */}
<ToggleSetting
@@ -188,9 +191,9 @@ export function CtaCard({
)}

<div className={`flex ${layout === 'immersive' ? 'flex-col' : 'flex-row'} gap-5 py-5 ${color === 'none' || hasSponsorLabel ? 'border-t border-grey-300 dark:border-grey-800' : ''} ${color === 'none' ? 'border-b border-grey-300 dark:border-grey-800' : 'mx-5'}`}>
{hasImage && (
{imageSrc && (
<div className={`block ${layout === 'immersive' ? 'w-full' : 'w-16 shrink-0'}`}>
<img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src="https://images.unsplash.com/photo-1511556532299-8f662fc26c06?q=80&w=4431&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D" />
<img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} />
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve accessibility for images and interactive elements.

  1. The image uses a generic "Placeholder" alt text which doesn't provide meaningful information for screen readers.
  2. Consider adding ARIA labels for interactive elements in the settings panel.
-            <img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} />
+            <img 
+                alt={buttonText || "CTA card image"} 
+                className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} 
+                src={imageSrc} 
+            />
📝 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
<img alt="Placeholder" className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`} src={imageSrc} />
<img
alt={buttonText || "CTA card image"}
className={`${layout === 'immersive' ? 'h-auto w-full' : 'aspect-square w-16 object-cover'} rounded-md`}
src={imageSrc}
/>

</div>
)}
<div className="flex flex-col gap-5">
@@ -248,8 +251,8 @@ CtaCard.propTypes = {
buttonText: PropTypes.string,
buttonUrl: PropTypes.string,
color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']),
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update color prop type to match available colors.

The color prop type validation doesn't match all colors defined in CALLOUT_COLORS. This could lead to runtime errors if pink or purple colors are used.

-    color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']),
+    color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red', 'pink', 'purple']),
📝 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
color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red']),
color: PropTypes.oneOf(['none', 'grey', 'white', 'blue', 'green', 'yellow', 'red', 'pink', 'purple']),

hasImage: PropTypes.bool,
hasSponsorLabel: PropTypes.bool,
hasSponsorLabel: PropTypes.bool,
imageSrc: PropTypes.string,
isEditing: PropTypes.bool,
isSelected: PropTypes.bool,
layout: PropTypes.oneOf(['minimal', 'immersive']),
@@ -259,7 +262,6 @@ CtaCard.propTypes = {
updateButtonText: PropTypes.func,
updateButtonUrl: PropTypes.func,
updateHasSponsorLabel: PropTypes.func,
updateHasImage: PropTypes.func,
updateShowButton: PropTypes.func,
updateLayout: PropTypes.func,
handleColorChange: PropTypes.func
@@ -269,14 +271,13 @@ CtaCard.defaultProps = {
buttonText: '',
buttonUrl: '',
color: 'none',
hasImage: false,
hasSponsorLabel: false,
imageSrc: '',
isEditing: false,
isSelected: false,
layout: 'immersive',
showButton: false,
updateHasSponsorLabel: () => {},
updateHasImage: () => {},
updateShowButton: () => {},
updateLayout: () => {},
handleColorChange: () => {}
Original file line number Diff line number Diff line change
@@ -93,8 +93,8 @@ Empty.args = {
display: 'Editing',
value: '',
showButton: false,
hasImage: false,
hasSponsorLabel: false,
imageSrc: '',
color: 'green',
layout: 'immersive',
buttonText: '',
@@ -107,8 +107,8 @@ Populated.args = {
display: 'Editing',
value: 'Introducing the Air Stride 90X – where bold design meets unmatched performance. Engineered for ultimate support and breathability, it’s the perfect companion for your active lifestyle. Step up your game, in style.',
showButton: true,
hasImage: true,
hasSponsorLabel: true,
imageSrc: 'https://images.unsplash.com/photo-1511556532299-8f662fc26c06?q=80&w=4431&auto=format&fit=crop&ixlib=rb-4.0.3&ixid=M3wxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHx8fA%3D%3D',
color: 'green',
layout: 'immersive',
buttonText: 'Grab 20% discount',
Original file line number Diff line number Diff line change
@@ -469,6 +469,7 @@ export function HeaderCard({alignment,
setFileInputRef={setFileInputRef}
size='xsmall'
src={backgroundImageSrc}
stacked={true}
onFileChange={onFileChange}
onRemoveMedia={() => {
handleClearBackgroundImage();
Original file line number Diff line number Diff line change
@@ -468,6 +468,7 @@ export function SignupCard({alignment,
setFileInputRef={setFileInputRef}
size='xsmall'
src={backgroundImageSrc}
stacked={true}
onFileChange={onFileChange}
onRemoveMedia={() => {
handleClearBackgroundImage();