[PULSE-36] feat: callout component for pages and issue descriptions#5856
[PULSE-36] feat: callout component for pages and issue descriptions#5856
Conversation
aaryan610
changed the title
WalkthroughThe pull request introduces several updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (2)
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 (
|
aaryan610
changed the title
aaryan610
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: 16
🧹 Outside diff range and nitpick comments (16)
packages/ui/src/emoji/helpers.ts (1)
Line range hint
1-11: Consider revising the type annotation and clarifying the function's intent.The addition of
.toString()on line 6 appears to be a defensive programming measure to ensure the input is always a string. While this can prevent potential runtime errors, there are a few points to consider:
- The function parameter is already typed as
string, so TypeScript should catch non-string inputs at compile-time.- If the intention is to handle non-string inputs, the type annotation should be updated to reflect this, e.g.,
(emoji: string | number).- The
parseIntcall later in the function would implicitly convert to string anyway, so this change might be redundant in some cases.To clarify the function's intent and improve type safety, consider one of these options:
- If the function should only accept strings:
export const emojiCodeToUnicode = (emoji: string) => { if (!emoji) return ""; // Rest of the function... }
- If the function should accept both strings and numbers:
export const emojiCodeToUnicode = (emoji: string | number) => { if (!emoji) return ""; const emojiString = emoji.toString(); // Rest of the function using emojiString... }Please clarify the intended behavior and update the function accordingly.
packages/editor/src/core/extensions/callout/read-only-extension.tsx (1)
7-14: LGTM: Extension is well-configured for read-only behavior.The
CustomCalloutReadOnlyExtensionis correctly set up to extendCustomCalloutExtensionConfigwith appropriate read-only behavior. The use ofReactNodeViewRendererfor rendering theCustomCalloutBlockis a good approach.Consider the following enhancements:
- Add error handling in the
addNodeViewmethod to gracefully handle potential rendering issues.- Implement additional configuration options (e.g., custom styling) to increase the extension's flexibility.
Example implementation:
addNodeView() { return ReactNodeViewRenderer(CustomCalloutBlock, { // Example error boundary errorFallback: () => <div>Error rendering callout</div>, // Example additional options additionalOptions: { customClass: 'read-only-callout', }, }); },packages/helpers/helpers/emoji.helper.ts (1)
1-22: Overall file review: Enhance robustness and clarity of emoji conversion utilitiesThis file provides useful utilities for emoji code conversions. However, there are common themes in both functions that could be addressed to improve the overall quality of the code:
- Function naming: Both function names could be more precise to accurately reflect their operations.
- Input validation: Both functions would benefit from stricter input validation to ensure they're working with the expected data formats.
- Error handling: Consider implementing consistent error handling across both functions.
- Code clarity: Remove redundant operations and unnecessary comments.
Implementing these suggestions will make the utility functions more robust, easier to understand, and less prone to unexpected behavior.
Consider creating a shared utility function for input validation and error handling that both main functions can use. This would promote code reuse and ensure consistent behavior across the module.
packages/editor/src/core/types/slash-commands-suggestion.ts (1)
26-27: LGTM! Consider grouping related commands.The additions of "background-color" and "callout" to the
TEditorCommandstype are correct and align well with the PR objectives. These changes support the new callout feature, allowing users to create callouts and customize their background color.For better code organization, consider grouping related commands together. For example, you could move "background-color" next to "text-color" and place "callout" with other block-level commands like "quote" or "code". This would improve readability and make it easier to maintain the list of commands in the future.
web/helpers/emoji.helper.tsx (3)
Line range hint
66-71: Approved: Improved input handling forconvertHexEmojiToDecimalThe addition of
.toString()enhances the function's robustness by ensuring the input is treated as a string. This change helps prevent potential errors when dealing with unexpected input types.Consider adding input validation to handle cases where
emojiUnifiedmight benullorundefined:export const convertHexEmojiToDecimal = (emojiUnified: string): string => { - if (!emojiUnified) return ""; + if (!emojiUnified || typeof emojiUnified !== 'string') return ""; return emojiUnified .toString() .split("-") .map((e) => parseInt(e, 16)) .join("-"); };
Line range hint
73-84: Approved: Enhanced input handling foremojiCodeToUnicodeThe addition of
.toString()improves the function's resilience by ensuring the input is treated as a string. This change helps prevent potential errors when dealing with unexpected input types.Consider adding input validation to handle cases where
emojimight benullorundefined:export const emojiCodeToUnicode = (emoji: string) => { - if (!emoji) return ""; + if (!emoji || typeof emoji !== 'string') return ""; // convert emoji code to unicode const uniCodeEmoji = emoji .toString() .split("-") .map((emoji) => parseInt(emoji, 10).toString(16)) .join("-"); return uniCodeEmoji; };
Line range hint
1-84: Summary: Improved robustness in emoji handling functionsThe changes in this file enhance the robustness of the
convertHexEmojiToDecimalandemojiCodeToUnicodefunctions by ensuring that inputs are treated as strings. These improvements align with the PR objectives by potentially supporting the new callout feature's emoji handling capabilities.Consider adding comprehensive input validation and error handling throughout the emoji helper functions to further improve the reliability and maintainability of the codebase. This could include:
- Consistent type checking for inputs
- Proper error messages for invalid inputs
- Handling of edge cases (e.g., empty strings, invalid emoji codes)
These enhancements would make the helper functions more resilient and easier to debug in the future.
packages/ui/src/emoji/logo.tsx (1)
Line range hint
9-19: Approve the type renaming and export.The renaming of
TLogoPropstoTEmojiLogoPropsand exporting it improves clarity and reusability. This change aligns well with the PR objectives by supporting the callout component feature across all editors.Consider adding a brief JSDoc comment to describe the purpose of this type, which could further enhance code readability and maintainability.
packages/editor/package.json (1)
40-40: Consider using a more specific version for "@plane/helpers".Using "*" as the version specifier for "@plane/helpers" may lead to unpredictable behavior and potential breaking changes in the future. Since this appears to be an internal package, consider using a more specific version or a version range that aligns with your development practices.
For example, you could use:
- "@plane/helpers": "*", + "@plane/helpers": "^0.1.0", // or whatever the current version isThis ensures better version control and reduces the risk of unexpected issues.
packages/editor/src/core/extensions/callout/color-selector.tsx (2)
22-73: LGTM with a minor suggestion: Component render logic is well-implemented.The render logic is well-structured and uses conditional rendering effectively. The use of the
cnutility for managing class names enhances maintainability. The component handles most accessibility concerns and provides a smooth user experience with animation classes.However, to further improve accessibility, consider adding an
aria-labelto the color selection buttons to describe the color for screen readers.Here's a suggested improvement for the color selection buttons:
<button key={color.key} type="button" className="flex-shrink-0 size-6 rounded border-[0.5px] border-custom-border-400 hover:opacity-60 transition-opacity" style={{ backgroundColor: color.backgroundColor, }} onClick={() => handleColorSelect(color.key)} + aria-label={`Select ${color.name} color`} />This change assumes that each color in
COLORS_LISThas anameproperty. If it doesn't, you may need to add this property to the color objects.
1-75: LGTM with optimization suggestion: Well-implemented color selector component.The
CalloutBlockColorSelectorcomponent is well-structured, follows React best practices, and aligns perfectly with the PR objectives. It provides a robust color selection feature for callouts, enhancing customization options across all editors.To potentially optimize performance, consider memoizing the component using
React.memo()if it's expected to be rendered frequently with the same props. This can help prevent unnecessary re-renders in the parent component.Example:
import React from 'react'; // ... (rest of the imports and component code) export const CalloutBlockColorSelector: React.FC<Props> = React.memo((props) => { // ... (component implementation) });This optimization is optional and its effectiveness depends on the specific use case and rendering frequency of this component within the larger application.
packages/ui/src/emoji/emoji-icon-picker-new.tsx (1)
114-114: Approved: Performance improvement with lazy loading emojisThe addition of the
lazyLoadEmojisprop to theEmojiPickercomponent is a positive change. This feature will likely improve the performance of the emoji picker by loading emojis on-demand, which can lead to faster initial render times and reduced memory usage, especially beneficial for devices with limited resources.This change aligns well with the PR's objective of enhancing the editor's functionality, particularly in managing callout blocks and emoji handling. It's a minimal change that doesn't introduce any apparent issues or side effects.
Consider monitoring the performance impact of this change, especially on slower devices or networks, to ensure it provides the expected benefits without introducing any noticeable delays in emoji rendering.
packages/editor/src/core/helpers/editor-commands.ts (1)
184-187: New insertCallout function addedThe
insertCalloutfunction is a great addition that aligns with the PR objectives of introducing a callout component for all editors. The implementation is consistent with other editor command functions in the file.For improved consistency with other functions, consider using a single-line implementation:
export const insertCallout = (editor: Editor, range?: Range) => { - if (range) editor.chain().focus().deleteRange(range).insertCallout().run(); - else editor.chain().focus().insertCallout().run(); + editor.chain().focus().deleteRange(range || {}).insertCallout().run(); };This change makes the function more concise and consistent with the style of other functions in the file.
packages/editor/src/styles/editor.css (1)
352-355: LGTM: Consistent spacing for h4, h5, and h6 with a suggestionThe changes to h4, h5, and h6 elements maintain the consistency established with higher-level headings, applying top margin only when not the first child. This improves overall document structure and spacing.
Consider slightly reducing the
margin-topvalue for h5 and h6 to further emphasize the heading hierarchy. For example:.prose :where(h5):not(:where([class~="not-prose"], [class~="not-prose"] *)) { &:not(:first-child) { - margin-top: 1rem; + margin-top: 0.9rem; } } .prose :where(h6):not(:where([class~="not-prose"], [class~="not-prose"] *)) { &:not(:first-child) { - margin-top: 1rem; + margin-top: 0.8rem; } }This subtle change would enhance the visual hierarchy of the document structure.
Also applies to: 363-366, 374-377
packages/editor/src/core/extensions/callout/extension.tsx (1)
56-57: Use a consistent error logging mechanismCurrently, errors are logged using
console.error. For better consistency and maintainability, consider using a centralized error handling or logging service if one is utilized in the project.Replace
console.errorwith the project's preferred error logging utility to maintain consistent error handling throughout the codebase.packages/editor/src/core/extensions/slash-commands/command-items-list.tsx (1)
190-190: Inconsistent parameter typing in 'command' functionsThe 'callout' command function includes explicit type annotations in its parameters:
command: ({ editor, range }: CommandProps) => insertCallout(editor, range),However, other command functions in the file do not include type annotations in their parameters. To maintain consistency across the codebase, consider removing the type annotation from the 'callout' command or adding type annotations to all command functions.
Apply this diff to remove the type annotation:
- command: ({ editor, range }: CommandProps) => insertCallout(editor, range), + command: ({ editor, range }) => insertCallout(editor, range),Alternatively, if you prefer to include type annotations for clarity, consider adding
: CommandPropsto all command function parameters.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
- packages/editor/package.json (2 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (2 hunks)
- packages/editor/src/core/extensions/callout/block.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/color-selector.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/extension-config.ts (1 hunks)
- packages/editor/src/core/extensions/callout/extension.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/index.ts (1 hunks)
- packages/editor/src/core/extensions/callout/logo-selector.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/read-only-extension.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/types.ts (1 hunks)
- packages/editor/src/core/extensions/callout/utils.ts (1 hunks)
- packages/editor/src/core/extensions/core-without-props.ts (2 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)
- packages/editor/src/core/extensions/slash-commands/command-items-list.tsx (5 hunks)
- packages/editor/src/core/helpers/editor-commands.ts (2 hunks)
- packages/editor/src/core/plugins/drag-handle.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (0 hunks)
- packages/editor/src/core/types/slash-commands-suggestion.ts (1 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- packages/helpers/helpers/emoji.helper.ts (1 hunks)
- packages/helpers/helpers/index.ts (1 hunks)
- packages/helpers/index.ts (1 hunks)
- packages/ui/src/emoji/emoji-icon-picker-new.tsx (1 hunks)
- packages/ui/src/emoji/helpers.ts (1 hunks)
- packages/ui/src/emoji/logo.tsx (2 hunks)
- packages/ui/src/emoji/lucide-icons-list.tsx (1 hunks)
- space/core/constants/editor.ts (2 hunks)
- web/core/constants/editor.ts (2 hunks)
- web/helpers/emoji.helper.tsx (2 hunks)
💤 Files with no reviewable changes (1)
- packages/editor/src/core/types/editor.ts
✅ Files skipped from review due to trivial changes (4)
- packages/editor/src/core/extensions/callout/index.ts
- packages/helpers/helpers/index.ts
- packages/helpers/index.ts
- packages/ui/src/emoji/lucide-icons-list.tsx
🧰 Additional context used
🔇 Additional comments (43)
packages/editor/src/core/extensions/callout/read-only-extension.tsx (2)
1-5: LGTM: Import statements are appropriate and concise.The import statements are well-organized and import only the necessary components and configurations for the extension. The use of path aliases (e.g., "@/extensions") is a good practice for maintaining clean and readable import statements.
1-14: Overall assessment: Well-implemented read-only extension for callout blocks.This new file successfully introduces the
CustomCalloutReadOnlyExtension, which aligns with the PR objectives of supporting callouts across all editors. The implementation is concise, follows good practices for Tiptap extensions, and correctly configures the read-only behavior.Key strengths:
- Proper extension of
CustomCalloutExtensionConfig- Correct configuration for read-only behavior
- Efficient use of
ReactNodeViewRendererfor rendering callout blocksWhile the current implementation is solid, consider the previously suggested enhancements for error handling and additional configuration options to further improve robustness and flexibility.
packages/editor/src/core/extensions/callout/types.ts (4)
1-8: LGTM: Well-structured enum for attribute names.The
EAttributeNamesenum is well-defined and follows good practices:
- Centralized constants improve maintainability.
- Naming convention adheres to TypeScript standards.
- Consistent use of all caps for enum values.
- Descriptive and consistently named values.
This enum will help prevent typos and improve code readability when working with callout block attributes.
10-13: LGTM: Well-defined type for callout block icon attributes.The
TCalloutBlockIconAttributestype is correctly defined:
- Uses
EAttributeNamesenum for property names, ensuring consistency.- Allows
string | undefinedfor both properties, appropriate for optional attributes.- Follows TypeScript naming conventions with the
Tprefix.This type will provide good type checking for icon-related attributes in callout blocks.
15-18: LGTM: Well-defined type for callout block emoji attributes.The
TCalloutBlockEmojiAttributestype is correctly defined:
- Uses
EAttributeNamesenum for property names, ensuring consistency.- Allows
string | undefinedfor both properties, appropriate for optional attributes.- Follows TypeScript naming conventions with the
Tprefix.- Maintains structural consistency with
TCalloutBlockIconAttributes.This type will provide good type checking for emoji-related attributes in callout blocks.
20-25: LGTM: Comprehensive type for callout block attributes with a minor consideration.The
TCalloutBlockAttributestype is well-defined:
- Correctly uses intersection to combine icon and emoji attributes.
- Appropriate use of
string | undefinedfor theclassproperty.- Good type safety with the union type for
LOGO_IN_USE.- Follows TypeScript naming conventions with the
Tprefix.One point to consider:
- The
BACKGROUNDproperty is required (not optional). Please verify if this is intentional, as it might be more flexible to allowundefinedfor cases where a default background could be used.Could you confirm if the
BACKGROUNDproperty should indeed be required? If it's intentional, no changes are needed. If not, consider updating it tostring | undefined.packages/ui/src/emoji/logo.tsx (1)
Line range hint
1-95: Overall assessment: Changes look good and align with PR objectives.The renaming and exporting of the
TEmojiLogoPropstype, along with its updated usage in thePropstype, contribute to better code organization and support for the callout component feature. These changes are minimal but effective, improving type consistency and potentially enhancing reusability across the codebase.packages/editor/package.json (2)
66-66: LGTM: Addition of prosemirror-utils.The addition of "prosemirror-utils" with version "^1.2.2" is appropriate. This library provides useful utilities for ProseMirror, which aligns well with the editor functionality of this package. The version specifier allows for compatible updates, which is a good practice for maintaining up-to-date dependencies while avoiding breaking changes.
40-40: Summary: Dependencies added for callout feature implementation.The additions of "@plane/helpers" and "prosemirror-utils" align well with the PR objectives of implementing a callout component for all editors. These dependencies likely provide necessary functionality for the new feature.
However, consider the following:
- Use a more specific version for "@plane/helpers" to ensure consistency.
- Verify that these new dependencies are utilized in the implementation of the callout feature in other files of this PR.
To ensure these dependencies are properly utilized, you can run the following script:
This script will search for imports of the new dependencies in the TypeScript and JavaScript files of the project, excluding package.json files.
Also applies to: 66-66
packages/editor/src/core/extensions/core-without-props.ts (3)
19-19: LGTM: New import for CustomCalloutExtensionConfigThe import statement for
CustomCalloutExtensionConfigis correctly added and follows the existing import style in the file.
19-19: Summary: Callout extension successfully integratedThe changes in this file successfully integrate the new callout extension into the editor's core extensions. The addition of
CustomCalloutExtensionConfigaligns well with the PR objectives of introducing a callout component for all editors. The implementation is clean, non-disruptive, and follows the existing code structure.A few points to consider:
- Ensure that the
CustomCalloutExtensionConfigis properly implemented and tested in its source file.- Verify that the callout functionality works as expected across all editors, as mentioned in the PR objectives.
- Consider adding documentation or comments explaining the purpose and usage of the new callout extension, if not already present in the source file.
Overall, these changes effectively support the new callout feature without disrupting the existing editor functionality.
Also applies to: 88-88
88-88: LGTM: CustomCalloutExtensionConfig added to CoreEditorExtensionsWithoutPropsThe
CustomCalloutExtensionConfigis correctly added to theCoreEditorExtensionsWithoutPropsarray. Its placement at the end of the array is appropriate and doesn't interfere with existing extensions.Let's verify the usage of
CustomCalloutExtensionConfigin other parts of the codebase:This script will help us ensure that
CustomCalloutExtensionConfigis properly used and imported in other relevant files.✅ Verification successful
Verified: CustomCalloutExtensionConfig Integration Confirmed
The
CustomCalloutExtensionConfighas been correctly added to theCoreEditorExtensionsWithoutPropsarray and is properly imported and referenced in relevant files. No issues were found with its integration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of CustomCalloutExtensionConfig in other files # Search for CustomCalloutExtensionConfig in all TypeScript and JavaScript files echo "Files referencing CustomCalloutExtensionConfig:" rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js -l 'CustomCalloutExtensionConfig' # Check if CustomCalloutExtensionConfig is imported in any file echo "\nImport statements for CustomCalloutExtensionConfig:" rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js 'import.*CustomCalloutExtensionConfig'Length of output: 1091
packages/editor/src/core/extensions/callout/color-selector.tsx (3)
1-12: LGTM: Imports and type definition are well-structured.The imports are appropriate for the component's needs, and the
Propstype is clearly defined with descriptive property names and types.
14-15: LGTM: Component declaration and props destructuring are well-implemented.The component is correctly declared as a React functional component with proper type annotations. The props destructuring enhances code readability.
17-20: LGTM: Color selection handler is implemented correctly.The
handleColorSelectfunction appropriately calls theonSelectprop with the selected color and toggles the dropdown visibility.space/core/constants/editor.ts (3)
18-18: LGTM: Import change looks good.The change from
QuotetoTextQuotein the imports is consistent with the update in the toolbar items.
78-78: LGTM: Toolbar item update is consistent.The change from
QuotetoTextQuotefor the "Quote" toolbar item is consistent with the import update. This change affects both "lite" and "document" editor types.To ensure the visual change is as expected, please run the following script:
Line range hint
1-105: Summary: Icon update completed, but clarification needed on relation to callout component.The changes in this file are consistent and well-implemented. The
Quoteicon has been replaced withTextQuotein both the imports and the toolbar items.However, I noticed that the PR objectives mention introducing a callout component, but this file doesn't contain any direct changes related to that feature. Could you please clarify how these icon changes relate to the new callout component feature? Are there other files in this PR that implement the callout functionality?
To help understand the broader context of these changes, please run the following script:
packages/editor/src/core/extensions/read-only-extensions.tsx (2)
24-24: LGTM: Import statement for CustomCalloutReadOnlyExtensionThe import statement for
CustomCalloutReadOnlyExtensionis correctly placed and consistent with the existing import style for other custom extensions.
128-128: LGTM: Addition of CustomCalloutReadOnlyExtensionThe
CustomCalloutReadOnlyExtensionis correctly added to the array of extensions returned byCoreReadOnlyEditorExtensions. This addition aligns with the PR objectives of introducing a callout component for all editors.To ensure the new extension is working as expected in read-only mode, please run the following verification script:
This script will help verify the implementation, usage, and testing of the new
CustomCalloutReadOnlyExtension.packages/editor/src/core/extensions/extensions.tsx (3)
11-11: LGTM: Import statement for CustomCalloutExtensionThe import statement for
CustomCalloutExtensionis correctly added to the existing import block from the local extensions. This change aligns with the existing code structure and import patterns.
Line range hint
1-161: Overall assessment: Changes look goodThe changes made to introduce the
CustomCalloutExtensionare well-integrated into the existing codebase. The import and usage of the new extension follow the established patterns in the file. These changes align with the PR objectives of introducing a callout component for all editors.To ensure full functionality:
- Verify that the callout feature works as expected in all supported editors.
- Check that the slash commands menu correctly includes the option to create callouts.
- Test the customization options for callouts, including emoji/icon changes and background color selection.
- Confirm that various components can be inserted within callouts.
- Validate that customization changes (emoji and background color) persist for future use.
158-158: LGTM: Addition of CustomCalloutExtensionThe
CustomCalloutExtensionis correctly added to the array of extensions returned byCoreEditorExtensions. Its placement in the array is logical, grouped with other custom extensions.To ensure the proper integration of the new callout feature, please verify:
- The callout functionality is accessible through the slash commands menu.
- Users can customize callouts by changing the emoji/icon and background color.
- Various components (headings, code blocks, quote blocks) can be inserted within the callout.
- Changes to emoji and background color are saved for future use.
You may want to run the following command to check for any additional configuration or usage of
CustomCalloutExtension:✅ Verification successful
LGTM: Addition of CustomCalloutExtension
The
CustomCalloutExtensionis correctly integrated into the codebase and properly included in the array of extensions returned byCoreEditorExtensions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg --type typescript --type javascript 'CustomCalloutExtension'Length of output: 98
Script:
#!/bin/bash # Searching for CustomCalloutExtension in TypeScript and JavaScript files rg --type ts --type js 'CustomCalloutExtension'Length of output: 1094
packages/editor/src/core/components/menus/menu-items.ts (3)
9-9: LGTM: Import statement updated correctly.The import statement has been updated to use
TextQuoteinstead ofQuoteIcon. This change aligns with the icon update in theQuoteItem.
Line range hint
1-248: Summary: Minor icon update enhances editor consistency.The changes in this file are minimal and focused, updating the quote icon from
QuoteIcontoTextQuote. This update:
- Maintains the existing functionality of the editor menu items.
- Potentially improves visual consistency or user experience.
- Aligns with the PR objectives of enhancing the editor's functionality.
The localized nature of these changes suggests a low risk of unintended side effects.
183-183: LGTM: Icon updated consistently.The icon for the
QuoteItemhas been updated fromQuoteIcontoTextQuote, which is consistent with the import statement change.To ensure this change doesn't affect other parts of the codebase, let's verify the usage of
TextQuote:✅ Verification successful
Verified: Icon update is consistent.
All instances of
QuoteIconhave been replaced withTextQuote, and there are no remaining references toQuoteIconin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of TextQuote icon across the codebase. # Test: Search for TextQuote usage. Expect: Only occurrences related to QuoteItem. rg --type typescript --type javascript 'TextQuote' # Test: Search for any remaining QuoteIcon usage. Expect: No results. rg --type typescript --type javascript 'QuoteIcon'Length of output: 171
Script:
#!/bin/bash # Description: Verify the usage of TextQuote icon across the codebase using file extensions. # Test: Search for TextQuote usage in TypeScript and JavaScript files. Expect: Only occurrences related to QuoteItem. rg --glob "*.ts" --glob "*.tsx" 'TextQuote' # Test: Search for any remaining QuoteIcon usage in TypeScript and JavaScript files. Expect: No results. rg --glob "*.ts" --glob "*.tsx" 'QuoteIcon'Length of output: 758
packages/editor/src/core/helpers/editor-commands.ts (2)
10-11: Improved setText function logicThe changes to the
setTextfunction enhance its behavior when a range is provided. Instead of clearing nodes, it now sets the node to "paragraph" after deleting the range. This ensures consistent paragraph formatting and aligns better with the function's purpose of setting text.
Line range hint
1-187: Overall assessment of changesThe modifications to the
setTextfunction and the addition of theinsertCalloutfunction align well with the PR objectives and enhance the editor's functionality. These changes improve the handling of text formatting and introduce support for callout components across all editors.The implementation is consistent with the existing codebase and follows established patterns. These updates will contribute to a more robust and feature-rich editing experience.
web/core/constants/editor.ts (3)
20-20: LGTM: Import statement updated correctly.The import statement has been updated to include
TextQuoteinstead ofQuote. This change is consistent with the modification in theUSER_ACTION_ITEMSarray and aligns with the PR objectives of enhancing the editor's functionality.
88-88: LGTM: USER_ACTION_ITEMS array updated correctly.The
USER_ACTION_ITEMSarray has been updated to useTextQuoteinstead ofQuotefor the "Quote" action. This change is consistent with the import statement update and aligns with the PR objectives of enhancing the editor's functionality and user experience.
Line range hint
1-324: Overall assessment: Changes are consistent and align with PR objectives.The modifications in this file are minor but consistent. They update the icon used for the "Quote" action in the editor's toolbar from
QuotetoTextQuote. These changes align with the PR objectives of enhancing the editor's functionality and user experience. No issues or concerns were identified during the review.packages/editor/src/styles/editor.css (5)
319-322: LGTM: Improved h1 spacing consistencyThe addition of the
:not(:first-child)selector for applying top margin to h1 elements is a good improvement. It ensures better spacing consistency by adding margin only when the h1 is not the first element, preventing unnecessary space at the document start.
330-333: LGTM: Consistent improvement for h2 spacingThe changes to h2 elements mirror those made to h1, applying top margin only when the h2 is not the first child. This maintains consistency in the heading hierarchy and improves overall document spacing.
341-344: LGTM: Consistent spacing improvement for h3The changes to h3 elements follow the pattern established for h1 and h2, applying top margin only when not the first child. This maintains consistency across heading levels and contributes to improved document structure.
385-392: LGTM: Improved paragraph spacingThe changes to paragraph styles effectively improve spacing consistency:
- Adding a small
margin-topto non-first-child paragraphs ensures proper separation between consecutive paragraphs.- Setting
margin-top: 0for the first-child paragraph prevents unnecessary space at the beginning of a section.These modifications contribute to a cleaner and more consistent layout throughout the document.
319-392: Overall: Excellent improvements to document typography and spacingThe changes made to this CSS file demonstrate a systematic approach to improving document typography and spacing. By consistently applying the
:not(:first-child)selector to headings and paragraphs, the layout becomes more coherent and visually appealing. These modifications:
- Eliminate unnecessary spacing at the beginning of sections.
- Maintain appropriate spacing between elements.
- Preserve the visual hierarchy of headings.
These improvements will enhance the overall readability and structure of documents created with this editor.
packages/editor/src/core/extensions/callout/extension.tsx (3)
50-53: Verify the condition in the Backspace handlerThe condition
parentNodeDetails.node.content.size > 2may not accurately reflect the intended logic. Verify that this condition correctly determines when to move the text selection.Consider whether the content size check should be
> 0instead of> 2, depending on the structure of the node's content. Testing with callout blocks of various content lengths can help ensure this condition behaves as expected.
65-66: Verify the import path forCustomCalloutBlockEnsure that
CustomCalloutBlockis correctly imported from"@/extensions". Incorrect import paths can lead to module not found errors.Double-check that
CustomCalloutBlockis exported from"@/extensions"and that the import statement is correct. If necessary, update the import path to reflect the accurate location of the component.
45-46:⚠️ Potential issueEnsure correct context for
thisinside arrow functionThe use of
this.typeinside the arrow function may lead to context issues because arrow functions do not have their ownthisbinding.Consider using a regular function to ensure
thisrefers to the extension instance:- const isParentNodeCallout: Predicate = (node) => node.type === this.type; + const isParentNodeCallout: Predicate = function (node) { + return node.type === this.type; + };Likely invalid or redundant comment.
packages/editor/src/core/extensions/slash-commands/command-items-list.tsx (3)
64-64: Refactored 'text' command to use 'setText' helper functionGreat job simplifying the 'text' command by utilizing the
setTexthelper function. This enhances readability and maintainability.
162-162: Updated icon for 'quote' command to 'TextQuote'The icon for the 'quote' command has been updated to
TextQuote, which more accurately represents a quotation and improves the user interface.
183-191: Added new 'callout' commandThe new 'callout' command is a valuable addition, providing users with enhanced functionality to insert callouts. The properties and search terms are appropriately defined.
packages/editor/src/core/plugins/drag-handle.ts (1)
46-46: Appropriate addition of.editor-callout-componentselectorThe inclusion of
.editor-callout-componentin thegeneralSelectorsarray ensures that thenodeDOMAtCoordsfunction correctly identifies callout components when determining the node at specific coordinates, thereby enhancing the editor's ability to handle callout elements.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/helpers/helpers/string.helper.ts (2)
3-11: Improve JSDoc comment for clarity and accuracyThe JSDoc comment is a good start, but it could be improved for better clarity and accuracy:
- The description should specifically mention that it sanitizes HTML by removing all tags, not just "remove all the HTML tags".
- Add a description for the
htmlStringparameter.- Include a description of the return value.
- Update the example to use the correct function name
sanitizeHTMLinstead ofstripHTML.Here's a suggested improvement:
/** * Sanitizes an HTML string by removing all HTML tags and trimming whitespace. * @param {string} htmlString - The HTML string to be sanitized. * @returns {string} The sanitized and trimmed string. * @example * const html = "<p>Some text</p>"; * const text = sanitizeHTML(html); * console.log(text); // "Some text" */
12-15: LGTM: Well-implemented sanitization functionThe
sanitizeHTMLfunction is well-implemented:
- It correctly uses DOMPurify to remove all HTML tags.
- Trimming the result is a good practice.
- The function is concise and follows the Single Responsibility Principle.
Minor suggestion: Consider adding a return type annotation for improved type safety:
export const sanitizeHTML = (htmlString: string): string => { // ... (existing implementation) };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
- packages/editor/src/core/extensions/callout/extension-config.ts (1 hunks)
- packages/editor/src/core/extensions/callout/logo-selector.tsx (1 hunks)
- packages/editor/src/core/extensions/callout/utils.ts (1 hunks)
- packages/helpers/helpers/index.ts (1 hunks)
- packages/helpers/helpers/string.helper.ts (1 hunks)
- packages/helpers/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/editor/src/core/extensions/callout/extension-config.ts
- packages/editor/src/core/extensions/callout/logo-selector.tsx
- packages/helpers/helpers/index.ts
🧰 Additional context used
🔇 Additional comments (6)
packages/helpers/package.json (1)
22-22: New dependency added: isomorphic-dompurifyThe addition of
isomorphic-dompurifysuggests an implementation of HTML sanitization, which is a good security practice. This aligns with enhancing the editor's functionality, particularly for handling user-generated content in callout blocks.Let's verify the usage of this new dependency in the codebase:
Please ensure that this new dependency is being utilized effectively in the codebase, particularly in areas related to the new callout component.
✅ Verification successful
Verification Successful: isomorphic-dompurify is utilized correctly
The
isomorphic-dompurifydependency is properly imported and itssanitizemethod is being used in the following files:
packages/helpers/helpers/string.helper.tsweb/helpers/string.helper.tsspace/helpers/string.helper.tsThis confirms that the new dependency aligns with the project's objectives for enhancing HTML sanitization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for the usage of isomorphic-dompurify in the codebase # Test: Search for import statements of isomorphic-dompurify echo "Searching for import statements of isomorphic-dompurify:" rg --type typescript --type javascript 'import.*from.*isomorphic-dompurify' # Test: Search for require statements of isomorphic-dompurify echo "Searching for require statements of isomorphic-dompurify:" rg --type typescript --type javascript 'require\(.*isomorphic-dompurify.*\)' # Test: Search for DOMPurify usage echo "Searching for DOMPurify usage:" rg --type typescript --type javascript 'DOMPurify\.sanitize'Length of output: 643
Script:
#!/bin/bash # Description: Check for the usage of isomorphic-dompurify in the codebase # Test: Search for import statements of isomorphic-dompurify echo "Searching for import statements of isomorphic-dompurify:" rg --type ts --type js 'import.*from.*isomorphic-dompurify' # Test: Search for require statements of isomorphic-dompurify echo "Searching for require statements of isomorphic-dompurify:" rg --type ts --type js 'require\(.*isomorphic-dompurify.*\)' # Test: Search for DOMPurify usage echo "Searching for DOMPurify usage:" rg --type ts --type js 'DOMPurify\.sanitize'Length of output: 1208
packages/helpers/helpers/string.helper.ts (2)
1-1: LGTM: Appropriate import for DOMPurifyThe import statement for
DOMPurifyfrom "isomorphic-dompurify" is correct and well-chosen. This package ensures that the sanitization works consistently across different JavaScript environments.
1-15: Overall assessment: Well-implemented sanitization helperThis new
sanitizeHTMLfunction is a valuable addition to the string helpers. It's well-implemented, using appropriate libraries and following good practices. The function aligns with the PR objectives by potentially supporting the processing of user input in the new callout component.A few minor improvements to the JSDoc comment and type annotation have been suggested, but these don't impact the functionality. Great job on this addition!
packages/editor/src/core/extensions/callout/utils.ts (3)
1-11: LGTM: Imports and type definitions are appropriate.The imports and type definitions are well-structured and relevant to the file's functionality.
26-62: LGTM: Robust implementation of getStoredLogo function.The function is well-implemented with several notable improvements:
- Proper error handling for JSON parsing.
- Sanitization of retrieved data to prevent XSS vulnerabilities.
- Appropriate fallback values when no valid data is found.
- Correct handling of browser vs. non-browser environments.
These improvements address the concerns raised in previous review comments. Good job on implementing these security and robustness enhancements.
69-84: LGTM: Well-implemented background color storage functions.Both
getStoredBackgroundColorandupdateStoredBackgroundColorfunctions are correctly implemented:
- They properly handle browser vs. non-browser environments.
getStoredBackgroundColorsanitizes the retrieved value, enhancing security.updateStoredBackgroundColorcorrectly handles null values, removing the item from localStorage when appropriate.- The condition in
updateStoredBackgroundColoruses strict equality (===), addressing the concern raised in a previous review comment.These implementations demonstrate good attention to detail and security considerations.
| export const DEFAULT_CALLOUT_BLOCK_ATTRIBUTES: TCalloutBlockAttributes = { | ||
| "data-logo-in-use": "emoji", | ||
| "data-icon-color": null, | ||
| "data-icon-name": null, | ||
| "data-emoji-unicode": "128161", | ||
| "data-emoji-url": "https://cdn.jsdelivr.net/npm/emoji-datasource-apple/img/apple/64/1f4a1.png", | ||
| "data-background": null, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a configurable base URL for emoji assets.
The data-emoji-url is currently hardcoded. For better maintainability and flexibility, consider using a configurable base URL for emoji assets.
You could refactor this by:
- Defining a constant for the base URL.
- Constructing the full URL using the base URL and the emoji code.
Example:
const EMOJI_BASE_URL = 'https://cdn.jsdelivr.net/npm/emoji-datasource-apple/img/apple/64/';
export const DEFAULT_CALLOUT_BLOCK_ATTRIBUTES: TCalloutBlockAttributes = {
// ... other attributes
"data-emoji-unicode": "1f4a1",
"data-emoji-url": `${EMOJI_BASE_URL}1f4a1.png`,
// ... remaining attributes
};This approach would make it easier to update the emoji source in the future if needed.
| export const updateStoredLogo = (value: TEmojiLogoProps): void => { | ||
| if (typeof window === "undefined") return; | ||
| localStorage.setItem("editor-calloutComponent-logo", JSON.stringify(value)); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider sanitizing input before storing in localStorage.
While the function correctly checks for the browser environment, it doesn't sanitize the input value before storing it in localStorage. To enhance security and prevent potential XSS attacks, consider sanitizing the input.
You could modify the function to sanitize the input like this:
export const updateStoredLogo = (value: TEmojiLogoProps): void => {
if (typeof window === "undefined") return;
const sanitizedValue = JSON.parse(sanitizeHTML(JSON.stringify(value)));
localStorage.setItem("editor-calloutComponent-logo", JSON.stringify(sanitizedValue));
};This approach ensures that any potentially malicious content in the input is sanitized before being stored.
aaryan610
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
packages/ui/src/emoji/emoji-icon-helper.tsx (2)
46-46: LGTM! Consider adding JSDoc for the new property.The addition of the
searchDisabledproperty toTCustomEmojiPickertype is a good enhancement, allowing for more flexibility in the emoji picker component. This change aligns well with the PR objectives of improving customization options.Consider adding a JSDoc comment for the new property to improve code documentation:
/** If true, disables the search functionality in the emoji picker. */ searchDisabled?: boolean;
57-57: LGTM! Consider adding JSDoc for the new property.The addition of the
searchDisabledproperty toTIconsListPropstype is consistent with the change made toTCustomEmojiPickerand provides similar flexibility for the icons list component.Consider adding a JSDoc comment for the new property to improve code documentation:
/** If true, disables the search functionality in the icons list. */ searchDisabled?: boolean;packages/ui/src/emoji/emoji-icon-picker.tsx (2)
28-30: LGTM! Consider renamingsearchDisabledfor clarity.The addition of
searchDisabledandsearchPlaceholderprops enhances the component's flexibility. The default values ensure backward compatibility, which is good practice.Consider renaming
searchDisabledtodisableSearchfor better readability and consistency with React naming conventions. This would make the prop's purpose more immediately clear:- searchDisabled = false, + disableSearch = false,
128-128: LGTM! Consider passingsearchPlaceholderto IconsList.The
searchDisabledprop is correctly passed to the IconsList component. However, for consistency, consider also passing thesearchPlaceholderprop to the IconsList component if it supports such functionality.If the IconsList component supports a search placeholder, consider passing the
searchPlaceholderprop:<IconsList defaultColor={defaultIconColor} onChange={(val) => { onChange({ type: EmojiIconPickerTypes.ICON, value: val, }); if (closeOnSelect) handleToggle(false); }} searchDisabled={searchDisabled} + searchPlaceholder={searchPlaceholder} />packages/ui/src/emoji/lucide-icons-list.tsx (2)
35-51: LGTM: Conditional rendering implemented correctly.The conditional rendering of the search input based on the
searchDisabledprop is well-implemented. It effectively controls the visibility of the search functionality as intended.Consider extracting the search input JSX into a separate component or function for improved readability and maintainability. This would make the main component less cluttered and easier to understand at a glance.
110-110: LGTM: Text size override applied.The addition of
!text-xsenforces a specific text size, which can be useful for maintaining consistent typography.Consider adding a comment explaining why this override is necessary. It would help other developers understand the reasoning behind using an important flag for the text size.
packages/ui/src/emoji/icons-list.tsx (1)
45-61: LGTM: Conditional rendering implemented correctlyThe conditional rendering of the search input based on the
searchDisabledprop is implemented correctly. This change effectively allows the search functionality to be disabled as per the PR objectives.For improved readability, consider using early return or ternary operator:
if (searchDisabled) return null; return ( <div className="flex items-center px-2 py-[15px] w-full "> {/* ... existing search input code ... */} </div> );Or:
{!searchDisabled ? ( <div className="flex items-center px-2 py-[15px] w-full "> {/* ... existing search input code ... */} </div> ) : null}This could make the conditional logic more explicit and easier to understand at a glance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/editor/src/core/extensions/callout/logo-selector.tsx (1 hunks)
- packages/ui/src/emoji/emoji-icon-helper.tsx (2 hunks)
- packages/ui/src/emoji/emoji-icon-picker-new.tsx (3 hunks)
- packages/ui/src/emoji/emoji-icon-picker.tsx (3 hunks)
- packages/ui/src/emoji/icons-list.tsx (2 hunks)
- packages/ui/src/emoji/lucide-icons-list.tsx (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/extensions/callout/logo-selector.tsx
- packages/ui/src/emoji/emoji-icon-picker-new.tsx
🧰 Additional context used
🔇 Additional comments (7)
packages/ui/src/emoji/emoji-icon-helper.tsx (1)
Line range hint
1-101: Overall, the changes look good and align with the PR objectives.The additions of the
searchDisabledproperty to bothTCustomEmojiPickerandTIconsListPropstypes enhance the flexibility of the emoji and icon selection components. These changes support the PR's goal of improving customization options for the callout component.The modifications are backward-compatible and well-placed within the existing type definitions. Consider adding JSDoc comments for the new properties to improve code documentation, as suggested in the previous comments.
packages/ui/src/emoji/emoji-icon-picker.tsx (2)
111-112: LGTM! Props correctly passed to EmojiPicker.The
searchDisabledandsearchPlaceholderprops are correctly passed to the EmojiPicker component, allowing the search functionality to be controlled from the parent component. This enhances the flexibility of the CustomEmojiIconPicker component.
Line range hint
1-138: Overall, the changes align well with the PR objectives.The modifications to the
CustomEmojiIconPickercomponent enhance its flexibility by allowing control over the search functionality. This aligns with the PR objective of introducing a customizable callout component, as it provides more options for users to interact with the emoji and icon selection process.The changes maintain backward compatibility through the use of default values for new props, which is a good practice. The implementation is consistent with the AI-generated summary and contributes to the overall goal of improving the editor's functionality in managing callout blocks and emoji handling.
packages/ui/src/emoji/lucide-icons-list.tsx (2)
14-14: LGTM: New prop addition is well-implemented.The addition of the
searchDisabledprop with a default value offalseis a good approach. It allows for flexibility in disabling the search functionality when needed while maintaining backward compatibility.
109-109: LGTM: Styling change prevents icon shrinking.The addition of
flex-shrink-0to the InfoIcon is a good practice to maintain the icon's size consistency in flex layouts.Could you provide more context on why this change was necessary? Was there a specific layout issue that this addresses?
packages/ui/src/emoji/icons-list.tsx (2)
15-15: LGTM: New prop added for search disablingThe addition of the
searchDisabledprop with a default value offalseis a good implementation. It allows for conditional rendering of the search input while maintaining backward compatibility.
Line range hint
1-161: Overall assessment: Changes implement the new feature correctlyThe changes to the
IconsListcomponent successfully implement the ability to disable the search functionality as described in the PR objectives. The implementation is correct, follows React best practices, and maintains backward compatibility. No security issues or major concerns were identified.Here's a summary of the changes:
- A new
searchDisabledprop was added with a default value offalse.- The search input rendering is now conditional based on the
searchDisabledprop.These changes enhance the component's flexibility without introducing breaking changes to existing usage.
3 participants
What's new?
All editors now support callouts. Users can create callouts using the slash commands' menu.
What can be done on callouts-
Your emoji and background color changes will be saved for the next time you create a callout.
Media:
Screen.Recording.2024-10-17.at.19.24.19.mov
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style
Tests