[PE-67] fix: color extension not working on issue description and published page #5852
Conversation
WalkthroughThe pull request introduces significant changes to the color management system within the editor. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (5)
packages/editor/src/core/extensions/core-without-props.ts (1)
Line range hint
1-89: Summary: Successful consolidation of color extensions.The changes in this file effectively consolidate the color-related extensions into a single
CustomColorExtension. This refactoring should improve code organization and maintainability without affecting other functionality in the editor.To ensure a smooth transition:
- Verify that the new
CustomColorExtensionproperly handles both text and background color functionality.- Update any documentation or comments referencing the old color extensions.
- Consider running integration tests to confirm that color-related features still work as expected in the editor.
packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (1)
19-20: Improved color state management, consider adding error handlingThe changes enhance encapsulation by using the
isActivemethod fromTextColorItemandBackgroundColorItem. This is a good improvement that makes the code more modular and maintainable.Consider adding error handling to make the code more robust:
- const activeTextColor = COLORS_LIST.find((c) => TextColorItem(editor).isActive(c.key)); - const activeBackgroundColor = COLORS_LIST.find((c) => BackgroundColorItem(editor).isActive(c.key)); + const activeTextColor = COLORS_LIST.find((c) => { + try { + return TextColorItem(editor).isActive(c.key); + } catch (error) { + console.error('Error checking active text color:', error); + return false; + } + }); + const activeBackgroundColor = COLORS_LIST.find((c) => { + try { + return BackgroundColorItem(editor).isActive(c.key); + } catch (error) { + console.error('Error checking active background color:', error); + return false; + } + });This change will prevent potential runtime errors if
isActivemethod throws an exception.packages/editor/src/core/extensions/custom-color.ts (3)
2-2: Remove unnecessary comment.The comment
// constantson line 2 is redundant since the import statement is self-explanatory.Apply this diff to remove the comment:
import { Mark, mergeAttributes } from "@tiptap/core"; -// constants import { COLORS_LIST } from "@/constants/common";
50-68: SimplifyrenderHTMLfor thecolorattribute.The
renderHTMLmethod can be streamlined by avoiding unnecessary variable reassignment and making the code more concise.Consider this refactored version:
renderHTML: (attributes: { color: string }) => { const { color } = attributes; if (!color) { return {}; } - let elementAttributes: Record<string, string> = { + const elementAttributes: Record<string, string> = { "data-text-color": color, - }; - - if (!COLORS_LIST.find((c) => c.key === color)) { - elementAttributes = { - ...elementAttributes, - style: `color: ${color}`, - }; + ...(COLORS_LIST.find((c) => c.key === color) + ? {} + : { style: `color: ${color}` }), }; return elementAttributes; },
73-91: SimplifyrenderHTMLfor thebackgroundColorattribute.Similarly, the
renderHTMLmethod forbackgroundColorcan be made more concise.Consider this refactored version:
renderHTML: (attributes: { backgroundColor: string }) => { const { backgroundColor } = attributes; if (!backgroundColor) { return {}; } - let elementAttributes: Record<string, string> = { + const elementAttributes: Record<string, string> = { "data-background-color": backgroundColor, - }; - - if (!COLORS_LIST.find((c) => c.key === backgroundColor)) { - elementAttributes = { - ...elementAttributes, - style: `background-color: ${backgroundColor}`, - }; + ...(COLORS_LIST.find((c) => c.key === backgroundColor) + ? {} + : { style: `background-color: ${backgroundColor}` }), }; return elementAttributes; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (1 hunks)
- packages/editor/src/core/extensions/core-without-props.ts (2 hunks)
- packages/editor/src/core/extensions/custom-background-color.ts (0 hunks)
- packages/editor/src/core/extensions/custom-color.ts (1 hunks)
- packages/editor/src/core/extensions/custom-text-color.ts (0 hunks)
- packages/editor/src/core/extensions/extensions.tsx (2 hunks)
- packages/editor/src/core/extensions/index.ts (1 hunks)
- packages/editor/src/core/extensions/read-only-extensions.tsx (2 hunks)
💤 Files with no reviewable changes (2)
- packages/editor/src/core/extensions/custom-background-color.ts
- packages/editor/src/core/extensions/custom-text-color.ts
🧰 Additional context used
🔇 Additional comments (13)
packages/editor/src/core/extensions/index.ts (2)
14-14: LGTM: New custom color export added.The addition of the "./custom-color" export is consistent with the PR objective of fixing color extension issues. This change likely consolidates the custom color functionality into a single module, which is a good practice for maintainability.
Line range hint
1-26: Verify removal of custom background and text color exports.The AI summary mentions the removal of "./custom-background-color" and "./custom-text-color" exports. While these removals are not visible in the provided code snippet, they align with the consolidation of color functionality. Please ensure these exports have been removed as intended.
✅ Verification successful
Removal of custom background and text color exports confirmed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify removal of custom background and text color exports # Test: Search for custom background and text color exports rg --type typescript 'export \* from "\./custom-(background|text)-color"' packages/editor/src/core/extensions/index.ts # Expected result: No matches foundLength of output: 155
Script:
#!/bin/bash # Description: Verify removal of custom background and text color exports # Test: Search for custom background and text color exports without relying on type rg 'export \* from "\./custom-(background|text)-color"' packages/editor/src/core/extensions/index.tsLength of output: 102
packages/editor/src/core/extensions/core-without-props.ts (2)
87-87: LGTM: Array updated to use consolidated color extension.The addition of
CustomColorExtensionto theCoreEditorExtensionsWithoutPropsarray is consistent with the import changes and PR objectives.Let's verify the removal of old color extensions from the array:
#!/bin/bash # Description: Verify removal of old color extensions from the array # Test: Search for old color extensions in the array. Expect: No results. rg --type typescript 'CustomTextColorExtension|CustomBackgroundColorExtension' packages/editor/src/core/extensions/core-without-props.ts
19-19: LGTM: Import changes reflect extension consolidation.The addition of
CustomColorExtensionimport and the removal of separate text and background color extension imports align with the PR objectives. This consolidation should improve code organization and maintainability.Let's verify the removal of old imports:
packages/editor/src/core/extensions/read-only-extensions.tsx (1)
24-24: LGTM! Verify removal of old color extensions.The addition of
CustomColorExtensionaligns with the PR objectives and the AI summary, consolidating color-related functionality into a single extension. This change should improve the color management system within the editor.To ensure the complete removal of the old color extensions, please run the following script:
This script will help ensure that the old extensions have been completely removed and the new
CustomColorExtensionis properly integrated.Also applies to: 125-125
✅ Verification successful
Verified removal of old color extensions and addition of
CustomColorExtension.All old color extensions have been successfully removed, and the new
CustomColorExtensionis properly integrated across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the removal of old color extensions and their usages # Test 1: Check for any remaining imports of old color extensions echo "Checking for remaining imports of old color extensions:" rg --type typescript "import.*CustomTextColorExtension|import.*CustomBackgroundColorExtension" packages/editor/src # Test 2: Check for any remaining uses of old color extensions echo "Checking for remaining uses of old color extensions:" rg --type typescript "CustomTextColorExtension|CustomBackgroundColorExtension" packages/editor/src # Test 3: Verify the presence of the new CustomColorExtension echo "Verifying the presence of the new CustomColorExtension:" rg --type typescript "CustomColorExtension" packages/editor/srcLength of output: 743
Script:
#!/bin/bash # Description: Verify the removal of old color extensions and their usages with corrected file types # Test 1: Check for any remaining imports of old color extensions echo "Checking for remaining imports of old color extensions:" rg --type ts "import.*CustomTextColorExtension|import.*CustomBackgroundColorExtension" packages/editor/src rg --type tsx "import.*CustomTextColorExtension|import.*CustomBackgroundColorExtension" packages/editor/src # Test 2: Check for any remaining uses of old color extensions echo "Checking for remaining uses of old color extensions:" rg --type ts "CustomTextColorExtension|CustomBackgroundColorExtension" packages/editor/src rg --type tsx "CustomTextColorExtension|CustomBackgroundColorExtension" packages/editor/src # Test 3: Verify the presence of the new CustomColorExtension echo "Verifying the presence of the new CustomColorExtension:" rg --type ts "CustomColorExtension" packages/editor/src rg --type tsx "CustomColorExtension" packages/editor/srcLength of output: 1580
Script:
#!/bin/bash # Description: Verify the removal of old color extensions and their usages using glob patterns # Test 1: Check for any remaining imports of old color extensions echo "Checking for remaining imports of old color extensions:" rg "import.*CustomTextColorExtension|import.*CustomBackgroundColorExtension" packages/editor/src --glob "*.ts" --glob "*.tsx" # Test 2: Check for any remaining uses of old color extensions echo "Checking for remaining uses of old color extensions:" rg "CustomTextColorExtension|CustomBackgroundColorExtension" packages/editor/src --glob "*.ts" --glob "*.tsx" # Test 3: Verify the presence of the new CustomColorExtension echo "Verifying the presence of the new CustomColorExtension:" rg "CustomColorExtension" packages/editor/src --glob "*.ts" --glob "*.tsx"Length of output: 1292
packages/editor/src/core/extensions/extensions.tsx (3)
14-14: Approve the consolidation of color-related extensionsThe addition of
CustomColorExtensionand the removal ofCustomTextColorExtensionandCustomBackgroundColorExtension(as mentioned in the AI summary) indicate a consolidation of color-related functionality. This change aligns with the PR objective and could potentially improve code organization and maintainability.
157-157: Approve the addition of CustomColorExtensionThe addition of
CustomColorExtensionto the returned array of extensions is consistent with the import changes and aligns with the PR objective of fixing color extension issues.
156-156: Clarify the purpose ofincludeChildrenin Placeholder configurationThe addition of
includeChildren: trueto the Placeholder configuration might affect how placeholders are displayed. Could you please explain the reasoning behind this change and its expected impact on the editor's behavior?packages/editor/src/core/components/menus/menu-items.ts (3)
214-222: Summary of changes to color handling in menu itemsThe updates to both
TextColorItemandBackgroundColorItemrepresent a consistent change in how active colors are determined. These changes:
- Improve the integration with the custom color extension.
- Align with the PR objective of fixing color extension issues.
- Provide a more unified approach to checking active states for custom colors.
These modifications should resolve the issue with color extensions not working on issue descriptions and published pages, as mentioned in the PR title.
To ensure full compatibility, please verify that these changes work correctly with the
CustomColorExtensionmentioned in the PR summary.
222-222: Approve changes toBackgroundColorItem.isActivemethod.The updated implementation for checking active background color improves consistency with custom color handling. This change aligns with the PR objective of fixing color extension issues and is consistent with the
TextColorItemupdate.To ensure this change doesn't introduce any regressions, please verify that:
- Background colors are correctly highlighted in the editor menu.
- Selecting a background color properly updates the menu state.
Run the following script to check for any other occurrences of the old implementation:
#!/bin/bash # Search for any remaining instances of the old background color check rg --type typescript 'getAttributes\("textStyle"\)\.backgroundColor\s*===\s*color'
214-214: Approve changes toTextColorItem.isActivemethod.The updated implementation for checking active text color improves consistency with custom color handling. This change aligns with the PR objective of fixing color extension issues.
To ensure this change doesn't introduce any regressions, please verify that:
- Text colors are correctly highlighted in the editor menu.
- Selecting a text color properly updates the menu state.
Run the following script to check for any other occurrences of the old implementation:
✅ Verification successful
Verified removal of the old text color check.
The search confirmed that there are no remaining instances of
getAttributes("textStyle").color === colorin the codebase. The changes effectively remove the old implementation, ensuring consistency with the new custom color handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of the old text color check rg --type typescript 'getAttributes\("textStyle"\)\.color\s*===\s*color'Length of output: 119
Script:
#!/bin/bash # Search for any remaining instances of the old text color check in .ts and .tsx files rg 'getAttributes\("textStyle"\)\.color\s*===\s*color' --glob '*.ts' --glob '*.tsx'Length of output: 85
packages/editor/src/core/extensions/custom-color.ts (2)
110-111: Ensure consistent attribute merging inrenderHTML.The
renderHTMLmethod merges HTML attributes, but conflicts might arise if bothcolorandbackgroundColorare applied. Ensure that attributes are merged without overriding each other.Please confirm that the merging of attributes handles both
data-text-coloranddata-background-colorcorrectly. Test scenarios where both attributes are present.
128-130: Verify the unsetting behavior inunsetBackgroundColor.Ensure that setting
{ backgroundColor: null }effectively removes the background color mark from the selected text.Test the
unsetBackgroundColorcommand to confirm it behaves as expected in the editor. If issues arise, additional logic might be needed to remove the mark.
| setTextColor: | ||
| (color: string) => | ||
| ({ chain }) => | ||
| chain().setMark(this.name, { color }).run(), | ||
| unsetTextColor: | ||
| () => | ||
| ({ chain }) => | ||
| chain().setMark(this.name, { color: null }).run(), | ||
| setBackgroundColor: | ||
| (backgroundColor: string) => | ||
| ({ chain }) => | ||
| chain().setMark(this.name, { backgroundColor }).run(), | ||
| unsetBackgroundColor: | ||
| () => | ||
| ({ chain }) => | ||
| chain().setMark(this.name, { backgroundColor: null }).run(), | ||
| }; |
There was a problem hiding this comment.
Add validation for color inputs in commands.
The setTextColor and setBackgroundColor commands accept any string, which might lead to invalid CSS values or security issues like CSS injection.
Consider adding validation to ensure that:
- The color values are valid CSS color strings.
- Or the color values are within the predefined
COLORS_LIST.
This improves robustness and security.
|
|
||
| /** | ||
| * Unset the background color | ||
| * @example editor.commands.unsetBackgroundColorColor() |
There was a problem hiding this comment.
Fix typo in unsetBackgroundColor example.
There's an extra "Color" in the example usage of unsetBackgroundColor.
Apply this diff to correct the typo:
/**
* Unset the background color
- * @example editor.commands.unsetBackgroundColorColor()
+ * @example editor.commands.unsetBackgroundColor()
*/📝 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.
| * @example editor.commands.unsetBackgroundColorColor() | |
| * @example editor.commands.unsetBackgroundColor() |
| declare module "@tiptap/core" { | ||
| interface Commands<ReturnType> { | ||
| color: { | ||
| /** | ||
| * Set the text color | ||
| * @param {string} color The color to set | ||
| * @example editor.commands.setTextColor('red') | ||
| */ | ||
| setTextColor: (color: string) => ReturnType; | ||
|
|
||
| /** | ||
| * Unset the text color | ||
| * @example editor.commands.unsetTextColor() | ||
| */ | ||
| unsetTextColor: () => ReturnType; | ||
| /** | ||
| * Set the background color | ||
| * @param {string} backgroundColor The color to set | ||
| * @example editor.commands.setBackgroundColor('red') | ||
| */ | ||
| setBackgroundColor: (backgroundColor: string) => ReturnType; | ||
|
|
||
| /** | ||
| * Unset the background color | ||
| * @example editor.commands.unsetBackgroundColorColor() | ||
| */ | ||
| unsetBackgroundColor: () => ReturnType; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider command grouping under color.
Grouping the commands under color might lead to confusion or conflicts with existing commands. It's more intuitive to place the commands directly under Commands<ReturnType> or use a more specific group name like customColor.
Apply this diff to adjust the command grouping:
declare module "@tiptap/core" {
interface Commands<ReturnType> {
- color: {
+ customColor: {
/**
* Set the text color
* @param {string} color The color to set
* @example editor.commands.setTextColor('red')
*/
setTextColor: (color: string) => ReturnType;
/**
* Unset the text color
* @example editor.commands.unsetTextColor()
*/
unsetTextColor: () => ReturnType;
/**
* Set the background color
* @param {string} backgroundColor The color to set
* @example editor.commands.setBackgroundColor('red')
*/
setBackgroundColor: (backgroundColor: string) => ReturnType;
/**
* Unset the background color
* @example editor.commands.unsetBackgroundColor()
*/
unsetBackgroundColor: () => ReturnType;
};
}
}📝 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.
| declare module "@tiptap/core" { | |
| interface Commands<ReturnType> { | |
| color: { | |
| /** | |
| * Set the text color | |
| * @param {string} color The color to set | |
| * @example editor.commands.setTextColor('red') | |
| */ | |
| setTextColor: (color: string) => ReturnType; | |
| /** | |
| * Unset the text color | |
| * @example editor.commands.unsetTextColor() | |
| */ | |
| unsetTextColor: () => ReturnType; | |
| /** | |
| * Set the background color | |
| * @param {string} backgroundColor The color to set | |
| * @example editor.commands.setBackgroundColor('red') | |
| */ | |
| setBackgroundColor: (backgroundColor: string) => ReturnType; | |
| /** | |
| * Unset the background color | |
| * @example editor.commands.unsetBackgroundColorColor() | |
| */ | |
| unsetBackgroundColor: () => ReturnType; | |
| }; | |
| } | |
| declare module "@tiptap/core" { | |
| interface Commands<ReturnType> { | |
| customColor: { | |
| /** | |
| * Set the text color | |
| * @param {string} color The color to set | |
| * @example editor.commands.setTextColor('red') | |
| */ | |
| setTextColor: (color: string) => ReturnType; | |
| /** | |
| * Unset the text color | |
| * @example editor.commands.unsetTextColor() | |
| */ | |
| unsetTextColor: () => ReturnType; | |
| /** | |
| * Set the background color | |
| * @param {string} backgroundColor The color to set | |
| * @example editor.commands.setBackgroundColor('red') | |
| */ | |
| setBackgroundColor: (backgroundColor: string) => ReturnType; | |
| /** | |
| * Unset the background color | |
| * @example editor.commands.unsetBackgroundColor() | |
| */ | |
| unsetBackgroundColor: () => ReturnType; | |
| }; | |
| } |
pushya22
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Chores