Changed various UI components. #1451
Conversation
Ref https://linear.app/ghost/issue/PLG-355/improve-cta-card-settings-panel - Updated ButtonGroup styles. - Added a button-style image upload placeholder to MediaPlaceholder. Also updated the VideoCard to follow the same style. - Refactored ColorOptionButtons and ColorPicker to display swatches and colorpicker inside toolbar. This also affects the callout, signup and header cards and the image-background selector. - Added pink and purple as background colors to the CTA card. - Changed CTA card link styles to currentColor instead of accent color – both for the label and the body text.
WalkthroughThis update modifies several components across the project. In the Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/koenig-lexical/src/components/ui/ButtonGroup.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. packages/koenig-lexical/src/components/ui/ImageUploadForm.jsxOops! Something went wrong! :( ESLint: 8.57.0 ESLint couldn't find the config "react-app" to extend from. Please check that the name of the config is correct. The config "react-app" was referenced from the config file in "/packages/koenig-lexical/.eslintrc.cjs". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ronaldlangeveld
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (16)
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (1)
173-196: Consider deriving color PropTypes from CALLOUT_COLORS.Instead of manually listing color options, consider deriving them from the CALLOUT_COLORS object keys for better maintainability.
- color: PropTypes.oneOf(['white', 'grey', 'blue', 'green', 'yellow', 'red', 'pink', 'purple', 'accent']), + color: PropTypes.oneOf(Object.keys(CALLOUT_COLORS)),packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
9-9: Ensure documentation of thechildrenprop.
The newchildrenprop is helpful for injecting additional UI elements into the picker, but consider documenting its usage or providing a default behavior to help maintain clarity and consistency.
193-196: Consider giving users the option to auto-close the picker.
ThehandleColorPickerChangefunction currently avoids closing the popover, which can be helpful for making repeated selections. However, some users may expect it to close automatically upon color selection.
199-297: Avoid repetition of the conic gradient code.
The repeated conic gradient block (lines 210-216 and again at 276-282) could be factored out into a small helper or CSS class to improve readability and reduce duplication.- <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(...)', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - ... - }} /> + <div className="absolute inset-0 rounded-full p-[3px] my-conic-gradient" />packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (6)
20-30: Ensure usage oftypeprop is documented.
Although changing theCardTextcomponent to handle atypeprop is straightforward, consider adding documentation or a JSDoc comment indicating whentype='button'should be used. This will help maintain clarity for future contributors.
49-56: Consider simplifying top-level conditionals.
The classes for'button'vs.'border'usage might be compressed if the logic continues to grow. A dedicated helper or mapping object might improve readability and maintainability.
67-71: Styling consistency for small vs xsmall.
Ensure the text alignment and spacing remain consistent across all size variants, especially if new sizes or conditions are introduced.
87-88: Check container usage fordata-testid.
This is referencing the entire container as'media-placeholder'. If you need more granular UI tests, consider adding more descriptive naming for nested elements.
96-116: Separate the inline button styles from code logic.
Here, there's a direct style usage for'px-3 py-1'. To keep styling consolidated, consider using a helper class or reusingbuttonClasseswith extended conditions. Maintaining consistent logic prevents style drift.
117-139: Avoid large inline code blocks.
Both the “button” and “else” blocks contain a fair amount of inline HTML and classes. If you find the logic grows, consider refactoring them into smaller functional components with well-defined responsibilities.packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (1)
5-24: Well-structured new component and stylings.
ThisImageUploadSwatchis succinct and readable. Theoutline-greenstyle effectively highlights when the background image is active. If you plan to scale the palette, consider a more themable or dynamic approach to the outline color.packages/koenig-lexical/test/utils/background-color-helper.js (1)
1-28: Consider adding error handling for element locators.The implementation is well-structured and modular. However, it could benefit from error handling for cases where elements are not found.
Consider wrapping the locator operations in try-catch blocks:
export async function cardBackgroundColorSettings(page, {cardColorPickerTestId, customColor, colorTestId, findByColorTitle, imageUploadId, fireColorSetting = true}) { if (fireColorSetting) { - const colorSetting = page.locator(`[data-testid="${cardColorPickerTestId}"]`); - const colorButton = colorSetting.locator('button'); - await colorButton.click(); + try { + const colorSetting = page.locator(`[data-testid="${cardColorPickerTestId}"]`); + const colorButton = colorSetting.locator('button'); + await colorButton.click(); + } catch (error) { + throw new Error(`Failed to interact with color picker: ${error.message}`); + } } // Similar error handling for other operations... }packages/koenig-lexical/src/components/ui/ButtonGroup.jsx (1)
10-10: LGTM! Consider enhancing accessibility.The styling updates improve visual hierarchy and maintain dark mode support. The active state is now more prominent.
Consider adding ARIA attributes for better screen reader support:
- <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white"> + <ul className="flex items-center justify-evenly rounded-lg bg-grey-100 font-sans text-md font-normal text-white" role="toolbar" aria-label="Button group options">className={`group relative flex h-7 w-8 cursor-pointer items-center justify-center rounded-lg text-black dark:text-white dark:hover:bg-grey-900 ${isActive ? 'border border-grey-300 bg-white shadow-xs dark:bg-grey-900' : '' } ${Icon ? '' : 'text-[1.3rem] font-bold'}`} + aria-pressed={isActive}Also applies to: 36-36
packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
17-25: Consider memoizing the gradient style object.The gradient style object is recreated on every render. Consider memoizing it for better performance.
+const gradientStyle = { + background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', + WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + WebkitMaskComposite: 'xor', + mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', + maskComposite: 'exclude' +}; export function ColorOptionButtons({buttons = [], selectedName, onClick}) { // ... - <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={{ - background: 'conic-gradient(hsl(360,100%,50%),hsl(315,100%,50%),hsl(270,100%,50%),hsl(225,100%,50%),hsl(180,100%,50%),hsl(135,100%,50%),hsl(90,100%,50%),hsl(45,100%,50%),hsl(0,100%,50%))', - WebkitMask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - WebkitMaskComposite: 'xor', - mask: 'linear-gradient(#fff 0 0) content-box, linear-gradient(#fff 0 0)', - maskComposite: 'exclude' - }} /> + <div className="absolute inset-0 rounded-full bg-clip-content p-[3px]" style={gradientStyle} />packages/koenig-lexical/test/e2e/cards/header-card.test.js (1)
190-191: Update test assertions to match new implementation.The test assertions have been updated to check for
shadow-xsclass instead ofbg-grey-150, but the test description still refers to "size". Consider updating the test description to better reflect what's being tested.- test('can change the size', async function () { + test('can change the shadow size', async function () {packages/koenig-lexical/test/e2e/cards/signup-card.test.js (1)
364-365: Remove redundant comments.The commented code and redundant comments should be removed to maintain clean test code.
- // data-testid="settings-panel" - - await page.locator('[data-testid="settings-panel"]').click(); // click here to close the colour swatch + await page.locator('[data-testid="settings-panel"]').click();Also applies to: 374-377
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(2 hunks)packages/koenig-lexical/src/components/ui/ButtonGroup.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx(2 hunks)packages/koenig-lexical/src/components/ui/ColorPicker.jsx(5 hunks)packages/koenig-lexical/src/components/ui/ImageUploadForm.jsx(1 hunks)packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx(1 hunks)packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx(4 hunks)packages/koenig-lexical/src/components/ui/MediaUploader.jsx(3 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(3 hunks)packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx(8 hunks)packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v1/HeaderCard.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx(5 hunks)packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx(2 hunks)packages/koenig-lexical/src/components/ui/cards/VideoCard.jsx(2 hunks)packages/koenig-lexical/src/styles/components/kg-prose.css(1 hunks)packages/koenig-lexical/tailwind.config.cjs(1 hunks)packages/koenig-lexical/test/e2e/cards/callout-card.test.js(3 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(4 hunks)packages/koenig-lexical/test/e2e/cards/header-card.test.js(9 hunks)packages/koenig-lexical/test/e2e/cards/signup-card.test.js(10 hunks)packages/koenig-lexical/test/utils/background-color-helper.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/koenig-lexical/src/components/ui/cards/HeaderCard/v1/HeaderCard.jsx
🔇 Additional comments (52)
packages/koenig-lexical/src/components/ui/cards/CalloutCard.jsx (3)
9-35: LGTM! Well-structured color constants with proper dark mode support.The color constants are well-organized, with consistent patterns and proper handling of dark mode. The special case for accent color links is also handled appropriately.
37-83: LGTM! Improved color visibility with consistent border styling.The opacity increase from 15% to 20% enhances visibility while maintaining a subtle appearance. The border colors are consistently applied across all swatches, providing a cohesive look in both light and dark modes.
85-171: LGTM! Well-structured component with proper state management.The component follows React best practices with:
- Proper use of hooks for state management
- Clean handling of emoji picker state
- Clear separation of concerns between display and settings panel
packages/koenig-lexical/src/components/ui/ColorPicker.jsx (3)
2-2: Nice addition of hooks.
Importing additional React hooks (e.g.,useCallback,useState) looks appropriate here and aligns well with the usage throughout the file.
78-99: Validate event-blocking logic for accessibility.
UsingonMouseDownandonTouchStartto callstopPropagationprevents the color picker from closing when clicked internally, which is desirable behavior. However, remember to verify accessibility scenarios (e.g., keyboard/screen reader users) to ensure these added event handlers don’t create navigation hurdles.
138-179: Good modular approach for color popover state.
The introduction of separate states (isOpen,showColorPicker,showChildren) and a dedicatedhandleDocumentClickfor closing the menu on outside clicks is a standard and effective pattern for managing popovers.packages/koenig-lexical/src/components/ui/MediaPlaceholder.jsx (6)
9-9: Nice addition ofclsxfor conditional class handling.
This import is appropriate and simplifies the code.
37-37: Verify that alltypeenum values are handled.
The newtypeprop is introduced here. Double-check whether your code can gracefully handle future expansions (e.g.,'gallery'or others). If not, consider either refining the prop type or providing graceful fallbacks for unrecognized values.
58-65: Good usage ofclsxchaining.
This makes the icon styling easier to maintain. future changes can simply append or remove conditions here.
79-83: Enhanced error styling.
TheerrorClassesstructure is clear and ensures consistent styling. Consider whether more descriptive error messages are needed or if an icon might help highlight them further.
91-94: Eye-catching drag-over text.
The placeholder text "Drop 'em like it's hot" is fun. Ensure this is appropriate for all user contexts or consider i18n if needed.
151-151: PropType expansion fortype.
You've updated the propTypes to allow'image'and'button'. Ensure that any additional usage stays synchronized throughout the codebase (e.g., if'gallery'or'file'also become valid in the future).packages/koenig-lexical/src/components/ui/ImageUploadForm.jsx (1)
14-15:stopPropagation()usage check.
Stopping propagation is valid for preventing accidental clicks from bubbling. However, be mindful if you rely on any parent or global event listeners for distinct features (e.g., close modals, triggers). Confirm that halting the bubble won’t cause unexpected UI side effects.packages/koenig-lexical/src/components/ui/ImageUploadSwatch.jsx (1)
1-4: Comprehensive imports for new component.
Usingclsxand your custom icons/Tooltip is consistent with the project’s design system. Good approach.packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
11-14:❓ Verification inconclusive
LGTM! Consider version bump for breaking changes.
The default value changes improve the out-of-the-box experience. However, changing
showButtonfromfalsetotrueis a breaking change that might affect existing implementations.Please verify if this change requires a version bump by checking the package's versioning policy:
🏁 Script executed:
#!/bin/bash # Check current version and recent changes cat package.json | jq '.version' git log -1 --pretty=format:%BLength of output: 37
The root package.json returns a null version, indicating that versioning may be managed per package in this monorepo. Please run the above script to check the version in the specific package file. Once that information is confirmed according to the package’s versioning policy, verify whether the breaking change—changing
showButtonfromfalsetotrue—requires a version bump.Rewritten Review Comment:
Action Required: Verify package versioning for breaking change
The changes in the default values enhance the out-of-the-box experience. However, note that switching
showButtonfromfalsetotrueis a breaking change that might impact current integrations. Since the root package.json does not specify a version (it returnednull), please verify the version set inpackages/kg-default-nodes/package.jsonand confirm if a version bump is warranted by the package's versioning policy.
🏁 Script executed:
[run_scripts] #!/bin/bash if [ -f packages/kg-default-nodes/package.json ]; then echo "Checking version in packages/kg-default-nodes/package.json" cat packages/kg-default-nodes/package.json | jq '.version' else echo "Error: packages/kg-default-nodes/package.json not found" fiLength of output: 367
Action Required: Confirm appropriate version bump for breaking change
The updated defaults improve the initial experience, but note that changing
showButtonfromfalsetotrueis a breaking change that could impact existing integrations. The package's current version is"1.4.0"(as determined frompackages/kg-default-nodes/package.json). Please verify that this breaking change has been accompanied by an appropriate version bump in accordance with your semantic versioning policy.packages/koenig-lexical/src/components/ui/ColorOptionButtons.jsx (1)
7-9: LGTM! Clean state management implementation.The state management is well-implemented with proper initialization and selector for the selected button.
packages/koenig-lexical/src/components/ui/MediaUploader.jsx (1)
20-20: LGTM! Thetypeprop enhances the component's flexibility.The addition of the
typeprop with proper PropTypes validation aligns with the PR objectives to introduce a button-style image upload placeholder.Also applies to: 68-68, 125-125
packages/koenig-lexical/tailwind.config.cjs (1)
94-94: LGTM! The new box shadow utility enhances UI depth.The addition of the
xsbox shadow utility with a subtle shadow effect aligns with the PR objectives to enhance UI components.packages/koenig-lexical/src/components/ui/cards/VideoCard.jsx (1)
102-102: LGTM! The MediaUploadSetting changes improve user guidance.The addition of the 'Upload' description and button-style type enhances the user interface and aligns with the PR objectives.
Also applies to: 112-112
packages/koenig-lexical/src/components/ui/SettingsPanel.jsx (2)
246-273: LGTM! The ColorPickerSetting enhancements improve flexibility.The addition of
customToolbarContentandchildrenprops, along with simplified color picker integration, enhances component customization.
275-306: LGTM! The MediaUploadSetting layout improvements enhance adaptability.The conditional classes based on
typeandstackedprops provide better layout control, aligning with the PR objectives for UI enhancements.packages/koenig-lexical/test/e2e/cards/callout-card.test.js (4)
2-2: LGTM! Importing the background color helper.The addition of the
cardBackgroundColorSettingsimport improves code reusability by centralizing color setting logic.
128-136: Consider keeping the color picker test.While the test is commented out, it might be valuable to keep a test that verifies all colors are rendered in the picker, even if the implementation details have changed.
Do you want me to help generate an updated version of this test using the new
cardBackgroundColorSettingshelper?
138-144: LGTM! Improved test clarity and maintainability.The test has been updated to:
- Use a more descriptive name
- Utilize the centralized
cardBackgroundColorSettingshelper
315-319: LGTM! Improved test maintainability.The test has been updated to use the centralized
cardBackgroundColorSettingshelper, making it more maintainable.packages/koenig-lexical/src/components/ui/cards/CallToActionCard.jsx (7)
24-26: LGTM! Added new color options.The addition of pink and purple colors enhances the customization options available to users.
48-78: LGTM! Improved color contrast.The changes to color opacity values (from
/15to/20) improve visibility and contrast.
146-152: LGTM! Added test ID for better testability.The addition of the
dataTestIdprop improves the component's testability.
164-164: LGTM! Improved upload button UX.The changes to the
MediaUploadSettingcomponent:
- Add a descriptive label with
desc='Upload'- Use a more prominent button style with
type='button'Also applies to: 170-170
254-254: LGTM! Improved text styling.The updates to text styling classes:
- Add
text-prettyfor better text rendering- Use consistent color classes
Also applies to: 297-297
349-349: LGTM! Updated prop types.The
colorprop type has been updated to include the new pink and purple color options.
383-383: LGTM! Improved default UX.Setting
showButtontotrueby default improves the initial user experience by making the call-to-action more prominent.packages/kg-default-nodes/test/nodes/call-to-action.test.js (3)
86-88: LGTM! Updated test for new button visibility default.The test now correctly verifies that
showButtonistrueby default before testing the toggle functionality.
90-92: LGTM! Updated test for new button text default.The test now correctly verifies that
buttonTextdefaults to 'Learn more' before testing the change functionality.
102-104: LGTM! Updated test for new button color default.The test now correctly verifies that
buttonColordefaults to 'black' before testing the change functionality.packages/koenig-lexical/test/e2e/cards/cta-card.test.js (5)
122-129: LGTM! Improved button visibility testing.The test now correctly verifies that:
- The button is visible by default
- All button settings are visible by default
131-141: LGTM! Improved toggle testing.The test now properly verifies both the visibility and invisibility states of the button when toggled.
218-220: LGTM! Improved color change testing.The tests now use the centralized
cardBackgroundColorSettingshelper for better maintainability.Also applies to: 228-229, 236-237, 245-246
315-317: LGTM! Added tests for new colors.The test suite now includes verification of the new pink and purple color options.
323-323: LGTM! Improved color class testing.The test now:
- Verifies the absence of all color classes initially
- Uses a more maintainable approach to color selection
Also applies to: 325-326
packages/koenig-lexical/src/components/ui/cards/HeaderCard/v2/HeaderCard.jsx (4)
19-19: LGTM! Improved modularity with ImageUploadSwatch.The import of ImageUploadSwatch enhances code reusability by extracting common image upload functionality into a dedicated component.
231-235: LGTM! Well-structured event handler.The
onBackgroundImageClickHandlerfunction properly manages state transitions:
- Shows background image
- Expands background color picker
- Collapses button color picker
413-417: LGTM! Clean implementation of ImageUploadSwatch.The ImageUploadSwatch component is properly integrated with required props for background image handling.
446-472: LGTM! Improved conditional rendering.The MediaUploadSetting is now properly conditionally rendered based on layout and showBackgroundImage state, with comprehensive props for image handling.
packages/koenig-lexical/src/components/ui/cards/SignupCard.jsx (2)
415-417: LGTM! Improved color picker state management.The background color picker now expands when selecting the background image option, providing better UX by showing relevant settings immediately.
452-478: LGTM! Enhanced conditional rendering.The MediaUploadSetting is properly conditionally rendered with comprehensive props for image handling.
packages/koenig-lexical/test/e2e/cards/header-card.test.js (2)
3-3: LGTM! Enhanced test maintainability.The import of cardBackgroundColorSettings helper improves test maintainability by centralizing color setting logic.
226-227: LGTM! Improved test readability.The background color tests now use the cardBackgroundColorSettings helper, making the tests more maintainable and easier to understand.
Also applies to: 232-233, 238-239
packages/koenig-lexical/test/e2e/cards/signup-card.test.js (3)
3-3: LGTM! Enhanced test maintainability.The import of cardBackgroundColorSettings helper improves test maintainability by centralizing color setting logic.
206-209: LGTM! Clean test implementation.The button color test now uses the cardBackgroundColorSettings helper with clear parameters.
232-236: LGTM! Improved test readability.The background color test now uses the cardBackgroundColorSettings helper with clear parameters.
packages/koenig-lexical/src/styles/components/kg-prose.css (1)
984-991: CTA Styling Implementation is Accurate and Consistent.The new CSS rules for
.koenig-lexical-cta-label aand.koenig-lexical-cta-text acorrectly apply the specified text colors and underline decoration for normal and dark modes using Tailwind’s@applydirective. This aligns well with the updated UI requirements for call-to-action elements described in the PR objectives. Please verify these styles in both light and dark mode contexts to ensure complete visual consistency.
| const buttonClasses = clsx( | ||
| 'group flex cursor-pointer select-none items-center justify-center', | ||
| size === 'xsmall' && 'p-4', | ||
| size !== 'xsmall' && 'flex-col p-20' | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential duplication of button styling code.
This buttonClasses definition partially overlaps with the inline version in the type === 'button' condition below. To keep the codebase DRY, consider consolidating them.
| {/* Color options popover */} | ||
| {isOpen && ( | ||
| <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"> | ||
| <div className="flex"> | ||
| <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"> | ||
| {buttons.map(({label, name, color}) => ( | ||
| name !== 'image' ? | ||
| <ColorButton | ||
| key={`${name}-${label}`} | ||
| color={color} | ||
| label={label} | ||
| name={name} | ||
| selectedName={selectedName} | ||
| onClick={(title) => { | ||
| onClick(title); | ||
| setIsOpen(false); | ||
| }} | ||
| /> | ||
| : | ||
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | ||
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | ||
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | ||
| </span> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for the color options popover.
The color options popover needs proper ARIA attributes for better accessibility.
- {isOpen && (
- <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow">
+ {isOpen && (
+ <div
+ className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"
+ role="dialog"
+ aria-label="Color options"
+ >- <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white">
+ <ul
+ className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"
+ role="listbox"
+ aria-label="Available colors"
+ >📝 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.
| {/* Color options popover */} | |
| {isOpen && ( | |
| <div className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow"> | |
| <div className="flex"> | |
| <ul className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white"> | |
| {buttons.map(({label, name, color}) => ( | |
| name !== 'image' ? | |
| <ColorButton | |
| key={`${name}-${label}`} | |
| color={color} | |
| label={label} | |
| name={name} | |
| selectedName={selectedName} | |
| onClick={(title) => { | |
| onClick(title); | |
| setIsOpen(false); | |
| }} | |
| /> | |
| : | |
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | |
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | |
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | |
| </span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| </div> | |
| )} | |
| {/* Color options popover */} | |
| {isOpen && ( | |
| <div | |
| className="absolute -right-3 bottom-full z-10 mb-2 rounded-lg bg-white px-3 py-2 shadow" | |
| role="dialog" | |
| aria-label="Color options" | |
| > | |
| <div className="flex"> | |
| <ul | |
| className="flex w-full items-center justify-between rounded-md font-sans text-md font-normal text-white" | |
| role="listbox" | |
| aria-label="Available colors" | |
| > | |
| {buttons.map(({label, name, color}) => ( | |
| name !== 'image' ? | |
| <ColorButton | |
| key={`${name}-${label}`} | |
| color={color} | |
| label={label} | |
| name={name} | |
| selectedName={selectedName} | |
| onClick={(title) => { | |
| onClick(title); | |
| setIsOpen(false); | |
| }} | |
| /> | |
| : | |
| <li key='background-image' className={`mb-0 flex size-[3rem] cursor-pointer items-center justify-center rounded-full border-2 ${selectedName === name ? 'border-green' : 'border-transparent'}`} data-testid="background-image-color-button" type="button" onClick={() => onClick(name)}> | |
| <span className="border-1 flex size-6 items-center justify-center rounded-full border border-black/5"> | |
| <PlusIcon className="size-3 stroke-grey-700 stroke-2 dark:stroke-grey-500 dark:group-hover:stroke-grey-100" /> | |
| </span> | |
| </li> | |
| ))} | |
| </ul> | |
| </div> | |
| </div> | |
| )} |
| @@ -0,0 +1,28 @@ | |||
| export async function cardBackgroundColorSettings(page, {cardColorPickerTestId, customColor, colorTestId, findByColorTitle, imageUploadId, fireColorSetting = true}) { | |||
There was a problem hiding this comment.
I think the naming could do with some work here as it took me a while to work out what this helper is doing when looking at the tests and the options weren't clear
fireColorSetting-fireis usually used for events so it sounds like internal behaviours are leaking into the naming, it would be better if this describes what is actually happening as far as the tester is concernedfindByColorTitle- it sounds like this is finding and returning something but it's actually clicking a setting?customColor- from the naming it's not clear that the intention is to input a custom colourcolorTestId/imageUploadId- again, nothing here suggests that this is causing clicks, the naming suggests you're just providing some information
Maybe it would be better to have separate functions for specific actions?
utils/card-settings.js
selectNamedColor(testid, 'White')inputCustomColor(testid, '#ff0000')uploadImage(testid, filename)dragAndDropImage(testid, filename)
| const picker = page.locator(`[data-testid="color-picker-toggle"]`); | ||
| await picker.click(); | ||
| const colorInput = page.locator(`input[aria-label="Color value"]`); | ||
| await colorInput.click({clickCount: 3}); |
There was a problem hiding this comment.
Why does this need three clicks?
Code comments are really helpful for anything out of the ordinary like this to explain the why
Ref https://linear.app/ghost/issue/PLG-355/improve-cta-card-settings-panel
stopPropagationto Image Upload Form to prevent event bubbling issues.These updates enhance UI consistency, improve test reliability, and ensure smoother user interactions.