Added editable Sponsored Label to CTA Card (replace with new PR) #1437
Conversation
ref https://linear.app/ghost/issue/PLG-325/turn-sponsored-into-editable-text - Added `customSponsorLabel` node property that will allow us to customise the text of the Sponsor label
ref https://linear.app/ghost/issue/PLG-325/turn-sponsored-into-editable-text - Added a new node that would allow us to add customise the the Sponsored Label
WalkthroughThe changes introduce a new property, 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/TabView.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/VisibilitySettings.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/SettingsPanel.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 (
|
![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 (3)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js (1)
17-17: Consider separating styling from content in the default sponsor label.The default value couples presentation with content by including inline styles. Consider moving the styling to CSS classes for better maintainability.
- {name: 'sponsorLabel', default: '<p><span style="white-space: pre-wrap;">SPONSORED</span></p>'}, + {name: 'sponsorLabel', default: '<p><span class="kg-sponsor-text">SPONSORED</span></p>'},packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (1)
236-248: Consider removing autoFocus for better accessibility.Setting
autoFocus={true}on the sponsor label editor might not be ideal for accessibility as it can be disorienting for screen reader users. Consider removing it unless there's a specific requirement.<KoenigNestedEditor - autoFocus={true} dataTestId={'sponsor-label-editor'} hasSettingsPanel={true} initialEditor={sponsorLabelHtmlEditor} initialEditorState={sponsorLabelHtmlEditorInitialState} nodes='basic' textClassName={clsx( 'kg-prose w-full whitespace-normal bg-transparent font-sans !text-2xs text-grey-900/40 dark:text-grey-100/40' )} >packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
293-295: Improve the text clearing implementation.The current implementation uses a hardcoded loop to clear text by pressing backspace 9 times. This is fragile and could break if the default text length changes.
Consider using the editor's API to clear the text directly or determine the number of backspaces needed dynamically:
-// clear the default text by hitting backspace 9 times -for (let i = 0; i < 9; i++) { - await page.keyboard.press('Backspace'); -} +const defaultText = await sponsorEditor.textContent(); +for (let i = 0; i < defaultText.length; i++) { + await page.keyboard.press('Backspace'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/kg-default-nodes/lib/nodes/call-to-action/CallToActionNode.js(1 hunks)packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(3 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(10 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(3 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(5 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(3 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (5)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js (1)
23-23: LGTM! Consistent integration of the sponsor label.The changes correctly integrate the dynamic sponsor label across templates and dataset.
Also applies to: 51-51, 72-72
packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
16-17: LGTM! Well-structured implementation of the sponsor label editor.The changes follow the established patterns for HTML editors in the codebase, maintaining consistency in:
- Property declarations
- Editor setup
- Dataset handling
- JSON export
- Props passing
Also applies to: 51-53, 62-63, 81-87, 112-113
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (1)
233-235: LGTM!The styling changes for the sponsor label container are clean and maintain proper color contrast in both light and dark modes.
packages/kg-default-nodes/test/nodes/call-to-action.test.js (1)
31-31: LGTM!The test coverage for the sponsorLabel property is comprehensive, including:
- Default value verification
- Setter/getter functionality
- Data persistence
Also applies to: 65-65, 95-97
packages/koenig-lexical/test/e2e/cards/cta-card.test.js (1)
273-316: LGTM!The new E2E tests provide excellent coverage of the sponsor label functionality:
- Default state verification
- Toggle visibility
- Text modification
- Content editor placeholder and text modification
| React.useEffect(() => { | ||
| htmlEditor.setEditable(isEditing); | ||
| }, [isEditing, htmlEditor]); | ||
| }, [isEditing, htmlEditor, sponsorLabelHtmlEditor]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update useEffect to handle sponsorLabelHtmlEditor editability.
The sponsorLabelHtmlEditor is included in the dependency array but its editability is not being set. Consider updating the effect to handle both editors:
React.useEffect(() => {
htmlEditor.setEditable(isEditing);
+ sponsorLabelHtmlEditor?.setEditable(isEditing);
}, [isEditing, htmlEditor, sponsorLabelHtmlEditor]);📝 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.
| React.useEffect(() => { | |
| htmlEditor.setEditable(isEditing); | |
| }, [isEditing, htmlEditor]); | |
| }, [isEditing, htmlEditor, sponsorLabelHtmlEditor]); | |
| React.useEffect(() => { | |
| htmlEditor.setEditable(isEditing); | |
| sponsorLabelHtmlEditor?.setEditable(isEditing); | |
| }, [isEditing, htmlEditor, sponsorLabelHtmlEditor]); |
There was a problem hiding this comment.
No this is not desirable since we just want the htmlEditor to be set as isEditing upon initial render since editing Sponsored label is optional.
There was a problem hiding this comment.
@ronaldlangeveld I understand now - thank you for the clarification. You're right, and I'll retract my suggestion since the sponsor label's editability should be handled separately from the main content editor.
✏️ Learnings added
Learnt from: ronaldlangeveld
PR: TryGhost/Koenig#1437
File: packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx:0-0
Timestamp: 2025-02-06T04:48:22.549Z
Learning: In the CallToActionNodeComponent of Koenig, only the main content editor (htmlEditor) should be set as editable in the useEffect hook. The sponsor label editor's (sponsorLabelHtmlEditor) editability is handled separately as it's an optional feature.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| sponsorLabelHtmlEditor, | ||
| sponsorLabelHtmlEditorInitialState, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add default values for new props.
The new props sponsorLabelHtmlEditor and sponsorLabelHtmlEditorInitialState are correctly added to PropTypes but are missing from defaultProps. This could cause issues if the props are not provided.
Add the following to defaultProps:
CtaCard.defaultProps = {
// ... existing props ...
+ sponsorLabelHtmlEditor: null,
+ sponsorLabelHtmlEditorInitialState: null,
onRemoveMedia: () => {}
};Also applies to: 356-357
no issue - disables the `koenig-lexical` and `kg-prose` classes when set to `false` in order to allow more customised styling - defaults to `true` in order to preserve current behaviour
no issue - provides a means to override class names added to Lexical-controlled elements
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (1)
242-256: Enhance accessibility of the sponsor label editor.Consider adding ARIA attributes to improve accessibility:
<KoenigNestedEditor autoFocus={true} dataTestId={'sponsor-label-editor'} + aria-label="Sponsor label editor" + role="textbox" hasSettingsPanel={true} initialEditor={sponsorLabelHtmlEditor} initialEditorState={sponsorLabelHtmlEditorInitialState} initialTheme={sponsoredLabelTheme} nodes='basic' textClassName={clsx( 'not-kg-prose w-full whitespace-normal bg-transparent font-sans !text-2xs text-grey-900/40 dark:text-grey-100/40' )} useDefaultClasses={false} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/koenig-lexical/src/components/KoenigNestedEditor.jsx(4 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/koenig-lexical/src/components/KoenigNestedEditor.jsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (2)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
25-28: LGTM! Clean theme implementation.The theme implementation follows best practices by extending the default theme and using semantic color values with opacity.
77-78: Add missing props to defaultProps.The new props are correctly defined in PropTypes but are missing from defaultProps.
Also applies to: 364-365
| <KoenigNestedEditor | ||
| autoFocus={true} | ||
| dataTestId={'sponsor-label-editor'} | ||
| hasSettingsPanel={true} | ||
| initialEditor={sponsorLabelHtmlEditor} | ||
| initialEditorState={sponsorLabelHtmlEditorInitialState} | ||
| initialTheme={sponsoredLabelTheme} | ||
| nodes='basic' | ||
| textClassName={clsx( | ||
| 'not-kg-prose w-full whitespace-normal bg-transparent font-sans !text-2xs text-grey-900/40 dark:text-grey-100/40' | ||
| )} | ||
| useDefaultClasses={false} | ||
| > | ||
| <ReplacementStringsPlugin /> | ||
| </KoenigNestedEditor> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for editor initialization.
Consider implementing error boundaries or try-catch blocks to handle potential editor initialization failures gracefully.
+const [editorError, setEditorError] = useState(null);
+// Add error handling wrapper
+const handleEditorError = (error) => {
+ console.error('Sponsor label editor failed to initialize:', error);
+ setEditorError(error);
+};
+{editorError ? (
+ <div className="text-red-500 text-sm">
+ Failed to load sponsor label editor. Please try again.
+ </div>
+) : (
<KoenigNestedEditor
autoFocus={true}
dataTestId={'sponsor-label-editor'}
hasSettingsPanel={true}
initialEditor={sponsorLabelHtmlEditor}
initialEditorState={sponsorLabelHtmlEditorInitialState}
initialTheme={sponsoredLabelTheme}
nodes='basic'
textClassName={clsx(
'not-kg-prose w-full whitespace-normal bg-transparent font-sans !text-2xs text-grey-900/40 dark:text-grey-100/40'
)}
useDefaultClasses={false}
+ onError={handleEditorError}
>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (2)
244-258:⚠️ Potential issueAdd error handling for editor initialization.
The nested editor lacks error handling for initialization failures.
Add error handling as suggested in the previous review:
+const [editorError, setEditorError] = useState(null); +// Add error handling wrapper +const handleEditorError = (error) => { + console.error('Sponsor label editor failed to initialize:', error); + setEditorError(error); +}; +{editorError ? ( + <div className="text-red-500 text-sm"> + Failed to load sponsor label editor. Please try again. + </div> +) : ( <KoenigNestedEditor autoFocus={true} dataTestId={'sponsor-label-editor'} hasSettingsPanel={true} initialEditor={sponsorLabelHtmlEditor} initialEditorState={sponsorLabelHtmlEditorInitialState} initialTheme={sponsoredLabelTheme} nodes='basic' textClassName={clsx( 'not-kg-prose w-full whitespace-normal font-sans !text-xs font-semibold uppercase leading-8 tracking-normal text-grey-900/40 dark:text-grey-100/40' )} useDefaultClasses={false} + onError={handleEditorError} >
366-367:⚠️ Potential issueAdd default values for new props.
The new props
sponsorLabelHtmlEditorandsponsorLabelHtmlEditorInitialStateare missing from defaultProps.Add the following to defaultProps:
CtaCard.defaultProps = { // ... existing props ... + sponsorLabelHtmlEditor: null, + sponsorLabelHtmlEditorInitialState: null, onRemoveMedia: () => {} };Also applies to: 370-389
🧹 Nitpick comments (3)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx (3)
26-29: Consider using a more descriptive theme name.The theme name
sponsoredLabelThemecould be more descriptive, e.g.,sponsorLabelEditorTheme, to better indicate its purpose.-const sponsoredLabelTheme = { +const sponsorLabelEditorTheme = { ...defaultTheme, link: 'text-accent' };
252-254: Consider extracting text styles to a constant.The text styles for the sponsor label editor are complex and could be reused. Consider extracting them to a constant.
+const SPONSOR_LABEL_TEXT_STYLES = 'not-kg-prose w-full whitespace-normal font-sans !text-xs font-semibold uppercase leading-8 tracking-normal text-grey-900/40 dark:text-grey-100/40'; <KoenigNestedEditor // ... - textClassName={clsx( - 'not-kg-prose w-full whitespace-normal font-sans !text-xs font-semibold uppercase leading-8 tracking-normal text-grey-900/40 dark:text-grey-100/40' - )} + textClassName={SPONSOR_LABEL_TEXT_STYLES} // ... >
257-257: Consider adding a comment explaining the RestrictContentPlugin configuration.The
RestrictContentPluginconfiguration could benefit from a comment explaining why paragraphs are limited to 1 and whybrtags are disallowed.+// Restrict sponsor label to a single line without line breaks to maintain consistent styling <RestrictContentPlugin allowBr={false} paragraphs={1} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (1)
48-53: 💡 Verification agent🧩 Analysis chain
Verify the usage of the
onBlurprop.There's a TODO comment indicating that the
onBlurprop is not used byHtmlCard, yet it's being passed to the component. This inconsistency should be addressed.Let's verify if
HtmlCardactually uses theonBlurprop:Also applies to: 62-62
🏁 Script executed:
#!/bin/bash # Description: Check if HtmlCard component uses the onBlur prop # Search for HtmlCard component definition ast-grep --pattern 'export function HtmlCard({$$$}) { $$$ }' # Search for onBlur usage within HtmlCard rg -A 5 'onBlur' packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsxLength of output: 2087
Inconsistency Detected:
onBlurProp Is Unused inHtmlCard
- The HtmlNodeComponent passes an
onBlurprop to HtmlCard.- HtmlCard's implementation (lines 7-27 in
packages/koenig-lexical/src/components/ui/cards/HtmlCard.jsx) only acceptshtml,updateHtml,isEditing, anddarkMode—there is no handling or destructuring of anonBlurprop.- This confirms the inconsistency noted by the TODO comment. Please either remove the unused
onBlurprop from HtmlNodeComponent or update HtmlCard to accept and useonBlurif needed.
🧹 Nitpick comments (2)
packages/kg-default-nodes/lib/nodes/html/html-renderer.js (1)
47-55: Refine conditional blocks for readability
These nested conditions may be more readable if consolidated or split into named helper functions. Simplifying the branching could prevent future confusion.packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
96-96: Simplify the wrapperStyle prop.The ternary operator can be simplified to directly use the value.
- wrapperStyle={this.backgroundColor ? 'regular' : 'wide'} + wrapperStyle={this.backgroundColor || 'wide'}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js(4 hunks)packages/kg-default-nodes/lib/nodes/html/html-renderer.js(2 hunks)packages/kg-default-nodes/lib/utils/visibility.js(0 hunks)packages/kg-default-nodes/test/nodes/call-to-action.test.js(11 hunks)packages/kg-default-nodes/test/utils/visibility.test.js(1 hunks)packages/koenig-lexical/src/components/ui/SettingsPanel.jsx(1 hunks)packages/koenig-lexical/src/components/ui/TabView.jsx(2 hunks)packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx(1 hunks)packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx(6 hunks)packages/koenig-lexical/src/nodes/CallToActionNode.jsx(5 hunks)packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx(2 hunks)packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx(2 hunks)packages/koenig-lexical/test/e2e/cards/cta-card.test.js(3 hunks)packages/koenig-lexical/test/utils/e2e.js(0 hunks)
💤 Files with no reviewable changes (2)
- packages/koenig-lexical/test/utils/e2e.js
- packages/kg-default-nodes/lib/utils/visibility.js
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/koenig-lexical/test/e2e/cards/cta-card.test.js
- packages/kg-default-nodes/test/nodes/call-to-action.test.js
- packages/kg-default-nodes/lib/nodes/call-to-action/calltoaction-renderer.js
- packages/koenig-lexical/src/components/ui/cards/CtaCard.jsx
- packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx
🧰 Additional context used
🧠 Learnings (2)
packages/kg-default-nodes/test/utils/visibility.test.js (1)
Learnt from: kevinansfield
PR: TryGhost/Koenig#1424
File: packages/kg-default-nodes/lib/utils/visibility.js:14-18
Timestamp: 2025-01-28T18:55:16.157Z
Learning: The `usesOldVisibilityFormat` function in `visibility.js` already checks for the existence of the 'web' property using `Object.prototype.hasOwnProperty.call(visibility, 'web')` before accessing `visibility.web.nonMember`.
packages/koenig-lexical/src/nodes/CallToActionNode.jsx (1)
Learnt from: ronaldlangeveld
PR: TryGhost/Koenig#1437
File: packages/koenig-lexical/src/nodes/CallToActionNodeComponent.jsx:0-0
Timestamp: 2025-02-06T04:48:22.660Z
Learning: In the CallToActionNodeComponent of Koenig, only the main content editor (htmlEditor) should be set as editable in the useEffect hook. The sponsor label editor's (sponsorLabelHtmlEditor) editability is handled separately as it's an optional feature.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Node 20.11.1
🔇 Additional comments (16)
packages/kg-default-nodes/lib/nodes/html/html-renderer.js (4)
3-3: Confirm complete removal of old references
The import change fromrenderWithVisibilityto these new visibility helpers is good. Please verify that there are no remaining references to the old function or logic in this file or elsewhere in the codebase.
20-23: Double-check short-circuit logic
By returning early whencontentVisibilityornode.visibilityis absent, the subsequent email/web-based gating logic will be skipped. Verify that this is the intended design for all usage scenarios.
25-31: Ensure safe in-place migration
migrateOldVisibilityFormat(visibility)mutates thevisibilityobject. Confirm that any subsequent references tovisibilityaccount for the updated properties.
32-45: Handle additional email segments
Currently, onlyNO_MEMBERS_SEGMENTandALL_MEMBERS_SEGMENThave explicit returns. In other cases, a container is returned. Confirm that this fallback behavior covers all intended segment values (including custom ones).packages/kg-default-nodes/test/utils/visibility.test.js (1)
2-2: References to legacy rendering removed
Removing the import ofrenderWithVisibilityaligns with the new approach. All references to the legacy rendering function appear correctly cleaned up.packages/koenig-lexical/src/nodes/HtmlNodeComponent.jsx (2)
29-33: LGTM! Clean refactoring of settings tab content.The refactoring improves code organization by using a structured object to map tab IDs to their content, making it easier to add more settings tabs in the future.
103-103: LGTM! Consistent usage of the refactored settings tab content.The change correctly utilizes the new
settingsTabContentsobject in theSettingsPanelcomponent.packages/koenig-lexical/src/nodes/CallToActionNode.jsx (5)
16-17: LGTM!The new properties for the sponsor label editor follow the established naming convention and pattern.
45-45: LGTM!The sponsor label editor setup follows the established pattern and properly handles initialization with both existing and default values.
Also applies to: 51-53
62-63: LGTM!The sponsor label editor properties are correctly added to the dataset, following the established pattern.
81-87: LGTM!The sponsor label HTML export follows the established pattern and includes proper HTML cleaning.
112-113: LGTM!The sponsor label editor props are correctly passed to the component.
packages/koenig-lexical/src/components/ui/VisibilitySettings.jsx (2)
3-29: LGTM! Component logic remains solid.The component maintains good separation of concerns and proper prop handling. The mapping logic for visibility options and toggles is clean and efficient.
18-18: Consider maintaining test coverage with alternative selectors.The removal of
data-testidfrom the group wrapper div might impact test maintainability. While individual toggles still havedataTestIdprops (line 9), the group-level selection in tests might become more brittle if relying on class names.Let's verify the test coverage and usage:
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for tests that might be affected by the removal of data-testid rg -l "data-testid.*visibility.*group" --type testLength of output: 81
Below is an updated shell script that doesn’t rely on the
--type testflag. This script uses glob filtering (assuming test files have “test” in their filename) to search for references to bothVisibilitySettingsanddata-testidin potential test files. This will help us better verify whether test cases may be impacted by the removal of thedata-testidattribute on line 18.After executing this script, please review the output manually. If any tests rely on the presence of the
data-testidattribute for group wrappers, they may need updating to use alternative selectors (e.g., class names or role attributes) to maintain test coverage.If no references are found, it might indicate that either tests are not directly asserting on this DOM attribute or they target sub-elements still containing test IDs. In either case, manual verification is advised to ensure the change doesn’t break any tests.
Action: Please run the above script and manually verify the test coverage regarding the removed attribute.
🏁 Script executed:
[run_scripts] #!/bin/bash echo "Searching for test files referencing 'VisibilitySettings' (case-insensitive)..." rg -li "VisibilitySettings" --glob "*test*.js" echo "-----------------------------------------------------" echo "Searching for test files referencing 'data-testid' (case-insensitive)..." rg -li "data-testid" --glob "*test*.js"Length of output: 2029
Action Required: Verify Test Selectors for the Group Wrapper
The removed
data-testidon the group-level<div>might affect tests that depend on it. Although numerous test files usedata-testid(e.g., in several E2E tests), there’s no clear evidence from our searches that any test explicitly targets the removed attribute on the VisibilitySettings group wrapper. However, to avoid any unintended breakages, please manually verify that tests (especially in files likecta-card.test.jsand related E2E tests) still correctly select elements—updating selectors to use class names, roles, or other reliable identifiers if necessary.packages/koenig-lexical/src/components/ui/TabView.jsx (2)
4-48: LGTM! Component structure and implementation are solid.The TabView component is well-structured with:
- Clear prop types validation
- Efficient state management
- Accessible button elements
- Responsive styling with dark mode support
32-32: Consider keeping the data-testid attribute for test coverage.The removal of the data-testid attribute from the tab content wrapper might affect test coverage. While individual tabs still have data-testid attributes, having one on the content wrapper can be useful for testing the rendered content.
Let's check if there are any tests relying on this data-testid:
❌ Incorrect review comment
🏁 Script executed:
#!/bin/bash # Search for tests that might be using the removed data-testid rg -l "data-testid.*tab.*content" --type testLength of output: 76
I'll run an updated search focusing on common test file naming conventions (e.g., files containing "test" in their names) to verify if any tests reference the removed
data-testidattribute. Please see the command below:
🏁 Script executed:
#!/bin/bash # Searching for tests referencing data-testid with 'tab' and 'content' in typical test files rg -l "data-testid.*tab.*content" -g "*test*.[jJ]s*"Length of output: 54
No test references to the removed
data-testidattribute found.Our searches for any test files referencing the attribute (using both patterns for
data-testid.*tab.*contentand related hooks likefindByTestIdin common test file naming patterns) returned no matches. This indicates that the removal does not impact the current test coverage for the component in question. If future tests require a hook for element selection, we can reconsider—but as of now, the tests appear unaffected.Likely an incorrect or invalid review comment.
| @@ -264,7 +264,7 @@ export function ColorPickerSetting({label, isExpanded, onSwatchChange, onPickerC | |||
| document.removeEventListener('click', closePicker); | |||
| }; | |||
| } | |||
| }, [isExpanded, onTogglePicker]); | |||
| }, [isExpanded]); | |||
There was a problem hiding this comment.
Add back onTogglePicker to useEffect dependencies.
Removing onTogglePicker from the dependencies array could lead to stale closure issues if the function changes. Since onTogglePicker is used within the effect, it should be included in the dependencies array.
- }, [isExpanded]);
+ }, [isExpanded, onTogglePicker]);📝 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.
| }, [isExpanded]); | |
| }, [isExpanded, onTogglePicker]); |
ronaldlangeveld
changed the title
ronaldlangeveld
changed the title
|
Had a little screw up with the merge conflicts, resolved them over on this PR #1443 |
ref https://linear.app/ghost/issue/PLG-325/turn-sponsored-into-editable-text
sponsorLabelnode for customisable sponsor text.CallToActionNodeto supportsponsorLabelas an editable field.CtaCard.jsxto render the sponsored label dynamically.sponsorLabelis present or absent.