[PULSE-42] feat: text alignment for all editors #5847
Conversation
|
Warning Rate limit exceeded@aaryan610 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 49 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces a new dependency, Changes
Possibly related PRs
Suggested reviewers
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: 15
🧹 Outside diff range and nitpick comments (19)
packages/editor/src/core/extensions/text-align.ts (1)
1-6: LGTM! Consider enhancing code readability.The implementation of the
CustomTextAlignExtensionlooks good and aligns well with the PR objectives. It correctly configures the TextAlign extension with the required alignment options (left, center, right) and applies them to both heading and paragraph content types.To improve code readability and maintainability, consider using constants for the alignment options and content types. Here's a suggested refactor:
import TextAlign from "@tiptap/extension-text-align"; +const ALIGNMENT_OPTIONS = ["left", "center", "right"] as const; +const CONTENT_TYPES = ["heading", "paragraph"] as const; + export const CustomTextAlignExtension = TextAlign.configure({ - alignments: ["left", "center", "right"], - types: ["heading", "paragraph"], + alignments: ALIGNMENT_OPTIONS, + types: CONTENT_TYPES, });This change would make it easier to maintain and update the supported alignments and content types in the future if needed.
packages/editor/src/core/extensions/index.ts (1)
Line range hint
1-26: Consider sorting exports alphabeticallyTo improve readability and maintainability, consider sorting the exports alphabetically. This will make it easier to locate specific exports in the future.
Here's a script to sort the exports:
#!/bin/bash # Description: Sort exports alphabetically # Test: Sort exports and display the result sort -o sorted_exports.tmp packages/editor/src/core/extensions/index.ts cat sorted_exports.tmp rm sorted_exports.tmppackages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (2)
19-65: LGTM: Text alignment options are well-defined.The
textAlignmentOptionsarray is structured appropriately, covering left, center, and right alignments as specified in the PR objectives. Each option includes the necessary properties for rendering and handling user interactions.Consider extracting the alignment options ("left", "center", "right") into a constant array or enum to improve maintainability. This would allow for easier addition or modification of alignment options in the future. Here's a suggested implementation:
const ALIGNMENT_OPTIONS = ["left", "center", "right"] as const; type AlignmentOption = typeof ALIGNMENT_OPTIONS[number]; const textAlignmentOptions = ALIGNMENT_OPTIONS.map((alignment) => ({ itemKey: "text-align" as const, renderKey: `text-align-${alignment}`, icon: alignmentIcons[alignment], command: () => menuItem.command({ alignment }), isActive: () => menuItem.isActive({ alignment }), })); // Define alignmentIcons separately const alignmentIcons: Record<AlignmentOption, LucideIcon> = { left: AlignLeft, center: AlignCenter, right: AlignRight, };This refactoring would make the code more DRY and easier to maintain.
67-88: LGTM: Render method is well-implemented with a suggestion for improvement.The component's render method effectively creates buttons for each alignment option, applying appropriate styling and event handlers. The use of the
cnhelper function for conditional class names is a good practice.To improve accessibility, consider adding
aria-labelattributes to the buttons. This will provide more context for screen reader users. Here's a suggested modification:<button key={item.renderKey} type="button" + aria-label={`Align text ${item.renderKey.split('-').pop()}`} onClick={(e) => { item.command(); e.stopPropagation(); }} className={cn( "size-7 grid place-items-center rounded text-custom-text-300 hover:bg-custom-background-80 active:bg-custom-background-80 transition-colors", { "bg-custom-background-80 text-custom-text-100": item.isActive(), } )} > <item.icon className="size-4" /> </button>This change will make the component more accessible to users relying on screen readers.
space/core/components/editor/lite-text-editor.tsx (2)
36-38: LGTM! Consider using useMemo forisEmpty.The changes improve code readability and simplify the
editorRefderivation. Good job!Consider using
useMemoforisEmptyto optimize performance, especially ifprops.initialValueis complex orisCommentEmptyis computationally expensive:const isEmpty = useMemo(() => isCommentEmpty(props.initialValue), [props.initialValue]);
Line range hint
1-77: Overall improvements with some follow-up tasks.The changes to the
LiteTextEditorcomponent andIssueCommentToolbarimprove code structure and flexibility. However, there are a few follow-up tasks to consider:
- Complete the toolbar homogenization mentioned in the TODO comment.
- Update type definitions to remove the need for type assertions.
- Consider adding unit tests for the new
executeCommandlogic to ensure it works as expected with different types of items.- Review the
@plane/editorpackage to ensure all exported types and components are up-to-date with these changes.These changes seem to be part of a larger refactoring effort. Ensure that all related components and files are updated consistently to maintain the overall architecture integrity.
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (1)
23-23: Summary: Improved type safety for editor commandsThese changes enhance the type safety of the
BubbleMenuNodeSelectorcomponent, particularly for editor commands. This aligns well with the PR's objective of introducing text alignment features, as it provides a more robust foundation for implementing new editor functionalities.Suggestions for follow-up:
- Ensure that the
TEditorCommandstype includes the new text alignment commands.- Consider applying similar type improvements to other components in the editor package for consistency.
- Update the component's documentation to reflect the enhanced type safety, if applicable.
Also applies to: 34-34
packages/editor/src/core/extensions/extensions.tsx (1)
Line range hint
1-163: Overall assessment: Changes look goodThe changes to this file are minimal and focused, correctly implementing the addition of the
CustomTextAlignExtension. This aligns well with the PR objectives of introducing text alignment features. The implementation follows existing patterns in the codebase, which is good for consistency and maintainability.However, to ensure a comprehensive review:
- Please confirm that the
CustomTextAlignExtensionhas been properly implemented and tested in its own file.- Verify that the new text alignment features (Left, Center, Right) work as expected in the editor, both through the toolbar and the specified keyboard shortcuts (Cmd + Shift + L, Cmd + Shift + E, Cmd + Shift + R).
- Check if any updates to documentation or user guides are needed to reflect these new text alignment capabilities.
Consider adding a comment in the
CoreEditorExtensionsfunction to briefly explain the purpose ofCustomTextAlignExtensionfor future maintainers.web/core/components/pages/editor/header/toolbar.tsx (1)
Action Required: Resolve
@ts-expect-errorandTODOCommentsThe verification reveals that there are still two
@ts-expect-errorcomments and twoTODOcomments inweb/core/components/pages/editor/header/toolbar.tsx. Please address these to ensure type safety and complete the planned refactoring.
- Resolve Type Mismatches: Investigate and fix the type issues causing the
@ts-expect-errorcomments.- Address TODOs: Create and reference tickets for the TODO items to track their completion.
- Clean Up Code: Remove any unnecessary debugging statements or commented-out code if applicable.
Once these actions are completed, the codebase will be more robust and maintainable.
🔗 Analysis chain
Line range hint
1-163: Summary of changes and recommendationsThe changes in this file generally improve consistency in how toolbar items are handled and referenced. The use of
itemKey,renderKey, andextraPropshas been standardized across the component, which is a positive change.However, there are a few items that need attention:
- Resolve type mismatches: There are two instances of
@ts-expect-errorin the file. These should be investigated and resolved to ensure proper type safety.- Address TODO comments: Create tickets for the TODO items and reference them in the code comments.
- Clean up debugging code: Remove commented-out console.log statements if they're no longer needed.
Overall, once these issues are addressed, the code will be more robust and maintainable.
To ensure all instances of
@ts-expect-errorand TODO comments are addressed, run the following script:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining @ts-expect-error comments and TODOs in the file echo "Checking for @ts-expect-error comments:" rg "@ts-expect-error" web/core/components/pages/editor/header/toolbar.tsx echo "\nChecking for TODO comments:" rg "TODO:" web/core/components/pages/editor/header/toolbar.tsxLength of output: 508
packages/editor/src/core/components/menus/bubble-menu/root.tsx (1)
30-32: Approved: Improved basicFormattingOptions logicThe renaming of
itemstobasicFormattingOptionsenhances code readability. The conditional logic for includingCodeItemis a good practice, ensuring that appropriate formatting options are displayed based on the editor's state.Consider extracting the condition
props.editor.isActive("code")into a descriptive variable for improved readability:const isCodeModeActive = props.editor.isActive("code"); const basicFormattingOptions = isCodeModeActive ? [CodeItem(props.editor)] : [BoldItem(props.editor), ItalicItem(props.editor), UnderLineItem(props.editor), StrikeThroughItem(props.editor)];This minor refactoring would make the code even more self-explanatory.
packages/editor/src/core/hooks/use-editor.ts (1)
155-155: Simplified active state check: Approved with suggestionThis change simplifies the
isMenuItemActivemethod by passing the entirepropsobject to theisActivefunction. This refactoring aligns with the changes made toexecuteMenuItemCommand, providing consistency and improving maintainability.For even better consistency, consider renaming the
propsparameter in both methods to something more specific, likemenuItemProps. This would make it clearer that these are properties related to menu items.Here's a suggested change for both methods:
- executeMenuItemCommand: (props) => { - const { itemKey } = props; + executeMenuItemCommand: (menuItemProps) => { + const { itemKey } = menuItemProps; // ... rest of the method - item.command(props); + item.command(menuItemProps); - isMenuItemActive: (props) => { - const { itemKey } = props; + isMenuItemActive: (menuItemProps) => { + const { itemKey } = menuItemProps; // ... rest of the method - return item.isActive(props); + return item.isActive(menuItemProps);space/core/components/editor/toolbar.tsx (1)
36-36: Address the TODO comment regarding toolbar homogenizationThere's a TODO comment indicating that this code needs updating during the toolbar homogenization process. To prevent this from being overlooked, ensure that this task is prioritized and tracked appropriately.
Do you want me to create a GitHub issue to track this task or assist with the homogenization?
space/core/constants/editor.ts (2)
46-46: Consider using a more intuitive icon for 'Text'The
CaseSensitiveicon may not effectively represent the "Text" formatting option to users. Consider choosing an icon that more clearly symbolizes "Text" to enhance user experience.
61-61: Ensure alignment shortcuts do not conflict with existing shortcutsPlease verify that the new alignment shortcuts (
Cmd + Shift + L,Cmd + Shift + E,Cmd + Shift + R) do not conflict with existing editor or system shortcuts to ensure a smooth user experience.Also applies to: 72-72, 83-83
web/core/components/editor/lite-text-editor/toolbar.tsx (1)
71-71: Address the TODO: Update during toolbar homogenizationThere's a TODO comment indicating that this code should be updated as part of the toolbar homogenization process.
Would you like assistance in updating this code segment for toolbar homogenization? I can help refactor the code to align with the homogenization goals or open a GitHub issue to track this task.
packages/editor/src/core/components/menus/menu-items.ts (1)
Line range hint
231-255: Reorder Menu Items for Improved User ExperienceThe
TextAlignItemis currently added at the end of the menu items array. To enhance the user experience, consider positioning it alongside other text formatting options likeBoldItem,ItalicItem, andUnderlineItem.Apply this diff to reposition
TextAlignItem:UnderLineItem(editor), + TextAlignItem(editor), StrikeThroughItem(editor), BulletListItem(editor),web/core/constants/editor.ts (3)
40-47: Enhance Documentation forToolbarMenuItemThe
ToolbarMenuItemtype now includes a generic parameter<T extends TEditorCommands>and an optionalextraProps. Consider updating any associated documentation or comments to reflect these changes for better clarity for future developers.
122-123: Fix Indentation for ConsistencyThere is an indentation issue at line 122 and 123. Consistent indentation improves code readability.
Apply this diff to fix the indentation:
{ - itemKey: "strikethrough", - renderKey: "strikethrough", + itemKey: "strikethrough", + renderKey: "strikethrough",
66-119: Consider Internationalization for Keyboard ShortcutsWhile defining keyboard shortcuts, consider that some users might have different keyboard layouts or internationalization settings. It's important to ensure that the shortcuts are accessible and do not conflict with existing shortcuts in various locales.
📜 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 (25)
- packages/editor/package.json (1 hunks)
- packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1 hunks)
- packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (2 hunks)
- packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (2 hunks)
- packages/editor/src/core/components/menus/bubble-menu/root.tsx (4 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 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/text-align.ts (1 hunks)
- packages/editor/src/core/helpers/editor-commands.ts (1 hunks)
- packages/editor/src/core/hooks/use-editor.ts (2 hunks)
- packages/editor/src/core/types/editor.ts (2 hunks)
- packages/editor/src/core/types/slash-commands-suggestion.ts (1 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- space/core/components/editor/lite-text-editor.tsx (3 hunks)
- space/core/components/editor/toolbar.tsx (3 hunks)
- space/core/components/issues/peek-overview/comment/add-comment.tsx (1 hunks)
- space/core/constants/editor.ts (3 hunks)
- web/core/components/editor/lite-text-editor/lite-text-editor.tsx (3 hunks)
- web/core/components/editor/lite-text-editor/toolbar.tsx (3 hunks)
- web/core/components/pages/editor/header/color-dropdown.tsx (1 hunks)
- web/core/components/pages/editor/header/toolbar.tsx (6 hunks)
- web/core/constants/editor.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- space/core/components/issues/peek-overview/comment/add-comment.tsx
🧰 Additional context used
🪛 Biome
packages/editor/src/core/types/editor.ts
[error] 60-60: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (58)
packages/editor/src/core/extensions/text-align.ts (2)
1-6: Overall assessment: Good implementation with room for minor enhancementsThe
CustomTextAlignExtensionimplementation successfully introduces the required text alignment functionality, aligning well with the PR objectives. It provides a solid foundation for text alignment in various editor contexts.Key points:
- The implementation is correct and focused.
- It supports the required alignment options (left, center, right) for both headings and paragraphs.
- The code is clean and concise.
Suggested enhancements:
- Improve readability by using constants for alignment options and content types.
- Enhance extensibility by creating a function to generate the configured extension.
- Verify integration in different editor contexts to ensure comprehensive implementation.
These suggestions are minor improvements to an already solid implementation. Great job on introducing this feature!
1-6: Verify integration in different editor contextsThe
CustomTextAlignExtensionimplementation looks good and provides the required text alignment functionality. To ensure it fully meets the PR objectives, we should verify its integration in various editor contexts.Please run the following script to check if the
CustomTextAlignExtensionis being used in the relevant editor configurations for pages, issue descriptions, and comments:This will help ensure that the text alignment feature is properly integrated across all required editor instances.
packages/editor/src/core/types/slash-commands-suggestion.ts (3)
Line range hint
1-22: Summary: Centralization of types improves maintainability, but requires careful verification.The changes in this file are part of a larger refactoring effort to centralize type definitions, which can improve maintainability. However, to ensure a smooth transition, please:
- Verify that the new
TEditorCommandsdefinition in "@/types" includes all necessary command types.- Check for any usage of the removed
TColorEditorCommandsandTNonColorEditorCommandstypes in the codebase and update them accordingly.- Ensure that all parts of the codebase using
TEditorCommandsare compatible with its new definition.These steps will help maintain backward compatibility and prevent potential runtime errors.
Line range hint
6-22: Verify backward compatibility and usage of removed types.While
CommandPropsandISlashCommandItemremain unchanged, which is good for compatibility, we need to ensure:
- The new
TEditorCommandsdefinition includes all previously available command types.- There are no usages of the removed
TColorEditorCommandsandTNonColorEditorCommandstypes in the codebase.Please run the following script to check for potential issues:
#!/bin/bash # Description: Verify backward compatibility and usage of removed types # Test 1: Check for usage of removed types echo "Checking for usage of removed types:" rg --type typescript "TColorEditorCommands|TNonColorEditorCommands" # Test 2: Compare old and new TEditorCommands definitions echo "Comparing old and new TEditorCommands definitions:" git show HEAD:packages/editor/src/core/types/slash-commands-suggestion.ts | rg "export type TEditorCommands" -A 10 echo "---" rg --type typescript "export type TEditorCommands" -A 10If the script finds any usage of removed types or discrepancies in the
TEditorCommandsdefinition, please update the affected areas to maintain compatibility.
3-4: Approve centralization ofTEditorCommands, but verify its new definition.The centralization of
TEditorCommandsby importing it from "@/types" can improve maintainability. However, ensure that the new definition in "@/types" includes all necessary command types previously defined in this file.Please run the following script to verify the new
TEditorCommandsdefinition:✅ Verification successful
Verification Successful:
TEditorCommandsCentralized and Removed Unused TypesThe centralization of
TEditorCommandsby importing it from@/typeshas been successfully verified. Additionally,TColorEditorCommandsandTNonColorEditorCommandshave been completely removed from the codebase, ensuring no deprecated or unused types remain.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new TEditorCommands definition and its usage # Test 1: Check the new TEditorCommands definition echo "Checking TEditorCommands definition:" rg --type typescript "export type TEditorCommands" -A 10 # Test 2: Verify usage of TEditorCommands in the codebase echo "Checking TEditorCommands usage:" rg --type typescript "TEditorCommands"Length of output: 524
Script:
#!/bin/bash # Description: Verify the new TEditorCommands definition and its usage by searching .ts and .tsx files # Test 1: Check the new TEditorCommands definition echo "Checking TEditorCommands definition in .ts and .tsx files:" rg "export type TEditorCommands" --glob "*.ts" --glob "*.tsx" -A 10 # Test 2: Verify usage of TEditorCommands in the codebase echo "Checking TEditorCommands usage in .ts and .tsx files:" rg "TEditorCommands" --glob "*.ts" --glob "*.tsx"Length of output: 4530
Script:
#!/bin/bash # Description: Verify the removal of TColorEditorCommands and TNonColorEditorCommands # Test 1: Check for any remaining TColorEditorCommands definitions or usages echo "Checking for any occurrences of TColorEditorCommands in .ts and .tsx files:" rg "TColorEditorCommands" --glob "*.ts" --glob "*.tsx" # Test 2: Check for any remaining TNonColorEditorCommands definitions or usages echo "Checking for any occurrences of TNonColorEditorCommands in .ts and .tsx files:" rg "TNonColorEditorCommands" --glob "*.ts" --glob "*.tsx"Length of output: 440
packages/editor/src/core/extensions/index.ts (2)
19-19: LGTM: Addition of headers exportThe addition of the
./headersexport aligns with the PR objectives of enhancing text editing capabilities.
25-25: LGTM: Addition of text-align exportThe addition of the
./text-alignexport is consistent with the PR objectives of introducing text alignment features for various editors.packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (3)
1-12: LGTM: Imports and type definition are appropriate.The imports cover all necessary dependencies for the component, including the Tiptap
Editor, icons fromlucide-react, and custom components/helpers. ThePropstype is correctly defined with theeditorprop of typeEditor.
1-89: LGTM: Overall implementation meets PR objectives.The
TextAlignmentSelectorcomponent successfully implements the text alignment functionality as specified in the PR objectives. It provides a user-friendly interface for selecting text alignment options (left, center, right) and integrates well with the Tiptap editor.To ensure full compliance with the PR objectives, please verify the implementation of the keyboard shortcuts mentioned (Cmd + Shift + L for left, Cmd + Shift + E for center, and Cmd + Shift + R for right alignment). Run the following script to check for the keyboard shortcut implementations:
If the keyboard shortcuts are not found in the current file, they might be implemented in a parent component or a separate keyboard handler. Please ensure these shortcuts are properly implemented and connected to the alignment functions.
14-17: LGTM: Component definition and initial setup look good.The
TextAlignmentSelectoris correctly defined as a functional component with proper prop typing. Theeditorprop is properly destructured for use within the component.Please verify the implementation of the
TextAlignItemfunction to ensure it returns the expected structure for themenuItem. Run the following script to check its implementation:✅ Verification successful
Action Required: Verify the
TextAlignItemfunction implementation usingast-grep.Please run the following script to locate the
TextAlignItemfunction definition:[/run_scripts]
Action Required: Further verify the
TextAlignItemfunction implementation using an updated search pattern.Please run the following script to locate the
TextAlignItemfunction definition with alternative declaration styles:[/run_scripts]
Verified: The
TextAlignItemfunction is correctly implemented and located inmenu-items.ts.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of TextAlignItem function # Test: Search for the TextAlignItem function definition rg --type typescript -A 10 'function TextAlignItem'Length of output: 256
Script:
#!/bin/bash # Description: Locate the TextAlignItem function definition using ast-grep ast-grep --lang typescript --pattern 'function TextAlignItem($_) { $$$ }'Length of output: 75
Script:
#!/bin/bash # Description: Locate the TextAlignItem function definition using ast-grep with alternative patterns # Pattern 1: Function expression ast-grep --lang typescript --pattern 'const TextAlignItem = ($_): $_ => { $$$ }' # Pattern 2: Arrow function ast-grep --lang typescript --pattern 'const TextAlignItem = ($_): $_ => ($$$)'Length of output: 870
space/core/components/editor/lite-text-editor.tsx (2)
70-70: LGTM! Simplified prop passing.The direct passing of
editorRefimproves code readability and maintainability. Good job!
3-3: LGTM! Consider verifying unused imports.The import statement has been updated correctly. The removal of
TNonColorEditorCommandssuggests it's no longer used in this file.To ensure all imports are being used and no unnecessary imports remain, run the following script:
packages/editor/package.json (1)
51-51: LGTM! Verify compatibility with existing dependencies.The addition of "@tiptap/extension-text-align" is appropriate for implementing the text alignment features described in the PR objectives. The version "^2.8.0" allows for compatible updates, which is a good practice.
To ensure compatibility, please run the following script:
packages/editor/src/core/extensions/core-without-props.ts (3)
21-21: LGTM: New import statement for text alignment extension.The import statement for
CustomTextAlignExtensionis correctly added and follows the existing pattern in the file. The naming convention is consistent with other custom extensions.
21-21: Overall assessment: Changes align well with PR objectives.The addition of the
CustomTextAlignExtensionimport and its inclusion in theCoreEditorExtensionsWithoutPropsarray effectively implements the text alignment feature as described in the PR objectives. These changes are minimal and focused, maintaining the existing structure of the file while enhancing the editor's capabilities.The implementation supports the goal of introducing text alignment features (Left, Center, Right) for various editors within the application. To fully meet the PR objectives, ensure that:
- The toolbar in the editor interface includes options for text alignment.
- Keyboard shortcuts (
Cmd + Shift + L,Cmd + Shift + E,Cmd + Shift + R) are properly implemented for text alignment.These aspects might be handled in other files not included in this review.
Also applies to: 91-91
91-91: LGTM: Text alignment extension added successfully.The
CustomTextAlignExtensionis correctly added to theCoreEditorExtensionsWithoutPropsarray. This addition aligns with the PR objectives to introduce text alignment features for various editors.Could you confirm if any configuration options are needed for the
CustomTextAlignExtension? If not, that's fine, but it's worth verifying to ensure we're not missing any important settings.✅ Verification successful
Action Required: Update Shell Script with Correct Type for ripgrep
The initial script failed because
typescriptis not a recognized file type forrg. Please update the script to use the correct type identifier, such asts, and rerun it to verify if theCustomTextAlignExtensionhas any configuration options.
Verified: Configuration options found for
CustomTextAlignExtension.The
CustomTextAlignExtensionincludes configuration options foralignmentsandtypes, ensuring that it functions as intended and aligns with the PR objectives to introduce text alignment features.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if CustomTextAlignExtension has any configuration options # Test: Search for the CustomTextAlignExtension definition rg --type typescript -A 10 'export const CustomTextAlignExtension'Length of output: 103
Script:
#!/bin/bash # Description: Check if CustomTextAlignExtension has any configuration options # Test: Search for the CustomTextAlignExtension definition using the correct file type rg --type ts -A 10 'export const CustomTextAlignExtension'Length of output: 404
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (2)
23-23: LGTM: New type import enhances type safety.The addition of
TEditorCommandsimport from@/typesis a good practice for improving type safety and code organization.
34-34: Approve: Enhanced type safety for editor menu items.The updated type signature
EditorMenuItem<TEditorCommands>[]foritemsimproves type safety by specifying the command types associated with each menu item. This change aligns well with the newTEditorCommandsimport and likely contributes to a more robust typing system for the editor.To ensure consistency across the codebase, please run the following script to check for other occurrences of
EditorMenuItemthat might need similar updates:This will help identify any inconsistencies in the usage of
EditorMenuItemacross the project.✅ Verification successful
Approve: Enhanced type safety for editor menu items.
The updated type signature
EditorMenuItem<TEditorCommands>[]foritemsimproves type safety by specifying the command types associated with each menu item. This change aligns well with the newTEditorCommandsimport and contributes to a more robust typing system for the editor. Verified that allEditorMenuItemusages are consistent with the updated generic type parameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of EditorMenuItem that might need updating # Search for EditorMenuItem usage without generic type parameter echo "Searching for EditorMenuItem usage without generic type parameter:" rg --type typescript "EditorMenuItem(\s*\[|[^<])" -g '!node_modules' # Search for EditorMenuItem usage with generic type parameter echo "Searching for EditorMenuItem usage with generic type parameter:" rg --type typescript "EditorMenuItem\s*<" -g '!node_modules'Length of output: 483
Script:
#!/bin/bash # Description: Check for other occurrences of EditorMenuItem that might need updating # Search for EditorMenuItem usage without generic type parameter echo "Searching for EditorMenuItem usage without generic type parameter:" rg --type ts "EditorMenuItem(\s*\[|[^<])" -g '!node_modules' # Search for EditorMenuItem usage with generic type parameter echo "Searching for EditorMenuItem usage with generic type parameter:" rg --type ts "EditorMenuItem\s*<" -g '!node_modules'Length of output: 4686
packages/editor/src/core/extensions/read-only-extensions.tsx (2)
130-130: LGTM: CustomTextAlignExtension successfully addedThe addition of
CustomTextAlignExtensionto theCoreReadOnlyEditorExtensionsarray is correct and aligns with the PR objectives of introducing text alignment features for various editors. The import and placement of the extension are appropriate.
Line range hint
1-131: Verify completeness of text alignment implementationThe addition of
CustomTextAlignExtensionis a good start for implementing text alignment features. However, to ensure full functionality:
- Verify that
CustomTextAlignExtensionis properly configured to handle left, center, and right alignments.- Check if any UI components (e.g., toolbar buttons) need to be added or modified to utilize this extension.
- Ensure that keyboard shortcuts (
Cmd + Shift + L,Cmd + Shift + E,Cmd + Shift + R) are properly set up to work with this extension.- Consider adding unit tests to verify the behavior of the new text alignment features.
To check for any related changes, run the following script:
web/core/components/editor/lite-text-editor/lite-text-editor.tsx (4)
67-68: LGTM! Verify impact of variable relocation.The
isEmptyvariable has been moved within the component. While this doesn't change its functionality, it's important to ensure that this relocation doesn't affect any other part of the component that might have depended on its previous scope.To verify the impact of this change, run the following script:
#!/bin/bash # Description: Check usage of isEmpty variable in the component # Test: Look for uses of isEmpty variable rg --type typescript -A 5 '\bisEmpty\b' web/core/components/editor/lite-text-editor/lite-text-editor.tsx
69-69: LGTM! Ensure consistent usage ofeditorRef.The introduction of
editorRefsimplifies access to the editor's ref throughout the component. This is a good refactoring that improves code readability and type safety.To ensure
editorRefis used consistently, run the following script:#!/bin/bash # Description: Check usage of editorRef variable in the component # Test: Look for uses of editorRef and ref variables rg --type typescript -A 5 '\b(editorRef|ref)\b' web/core/components/editor/lite-text-editor/lite-text-editor.tsx
92-98: LGTM with suggestions. Address TODO and type assertion.The refactoring of
executeCommandsimplifies the command execution logic and aligns with the earlier introduction ofeditorRef. However, there are two points that need attention:
- There's a TODO comment on line 93 about toolbar homogenization. This should be addressed or tracked as a separate issue.
- There's a type assertion (
@ts-expect-error) on line 94. It's generally better to avoid type assertions if possible. Consider investigating why this type mismatch occurs and if it can be resolved without the assertion.To investigate the type mismatch, run the following script:
#!/bin/bash # Description: Investigate type mismatch in executeCommand # Test: Look for the definition of executeMenuItemCommand ast-grep --lang typescript --pattern 'executeMenuItemCommand($$$)' web/core/components/editor/lite-text-editor/lite-text-editor.tsx # Test: Look for the type definition of EditorRefApi rg --type typescript -A 10 'type EditorRefApi' web/core/components/editor/lite-text-editor/lite-text-editor.tsxWould you like assistance in resolving the type mismatch or creating an issue to track the TODO?
3-3: LGTM! Consider verifying unused imports.The import statement has been updated to include necessary types and components. This change aligns with the refactoring of editor-related entities.
To ensure all imports are being used and no unnecessary imports remain, run the following script:
packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (3)
Line range hint
1-108: Summary of changes: Improved color selection implementationThe changes in this file enhance the color selection functionality for both text and background colors. The new implementation uses a more structured approach to pass color parameters, which improves type safety and code clarity. These modifications align well with the PR objectives and contribute to the overall enhancement of the editor's features.
Key points:
- Consistent parameter passing for both text and background color selection.
- Improved type safety and API design.
- Changes are localized and don't introduce any breaking changes to the component's interface.
67-67: Approve the structured parameter passing for text color.The change to pass the color key as a named property in an object (
{ color: color.key }) improves type safety and clarity. This is a good practice for API design.To ensure consistency, let's verify the
TextColorItemimplementation:✅ Verification successful
Re-running the search using TypeScript file extensions.
To locate the
TextColorItemimplementation, let's search specifically within.tsand.tsxfiles.
Verification Successful:
TextColorItemSupports Structured ParameterThe
TextColorItemimplementation correctly accepts an object with acolorproperty, ensuring that the recent change enhances type safety and clarity without causing inconsistencies.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the TextColorItem implementation accepts the new parameter structure. # Test: Search for the TextColorItem definition rg --type typescript -A 10 'export const TextColorItem'Length of output: 98
Script:
#!/bin/bash # Description: Verify the TextColorItem implementation accepts the new parameter structure. # Test: Search for the TextColorItem definition using TypeScript file extensions. rg --glob '*.ts' --glob '*.tsx' -A 10 'export const TextColorItem'Length of output: 1111
90-90: Approve the consistent parameter passing for background color.The change to pass the color key as a named property in an object (
{ color: color.key }) for background color selection maintains consistency with the text color implementation. This unified approach improves the overall code quality and maintainability.To ensure consistency, let's verify the
BackgroundColorItemimplementation:✅ Verification successful
To accurately locate the
BackgroundColorItemimplementation, let's run a corrected search:
To successfully locate the
BackgroundColorItemimplementation, let's use file globs to target.tsand.tsxfiles:
Approve the consistent parameter passing for background color.
The
commandfunction inBackgroundColorItemcorrectly accepts an object with acolorproperty. Passing{ color: color.key }aligns with the expected parameter structure, ensuring consistency and maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the BackgroundColorItem implementation accepts the new parameter structure. # Test: Search for the BackgroundColorItem definition rg --type typescript -A 10 'export const BackgroundColorItem'Length of output: 98
Script:
#!/bin/bash # Description: Correctly search for the BackgroundColorItem definition in TypeScript files. # Test: Search for the BackgroundColorItem definition using the correct TypeScript file types. rg --type ts,tsx -A 10 'export const BackgroundColorItem'Length of output: 90
Script:
#!/bin/bash # Description: Search for the BackgroundColorItem definition in .ts and .tsx files. # Test: Search for the BackgroundColorItem definition using file globs. rg 'export const BackgroundColorItem' --glob '*.ts' --glob '*.tsx' -A 10Length of output: 1138
packages/editor/src/core/extensions/extensions.tsx (1)
21-21: LGTM: Import of CustomTextAlignExtensionThe addition of
CustomTextAlignExtensionto the import statement is correct and follows the existing naming convention for custom extensions. This change aligns with the PR objective of introducing text alignment features.web/core/components/pages/editor/header/color-dropdown.tsx (2)
12-19: Great improvement in type safety!The update to the
Propstype definition enhances type safety by restricting thekeyparameter in bothhandleColorSelectandisColorActiveto only "text-color" and "background-color". This change:
- Ensures that only specific command types can be passed as arguments.
- Aligns with the updated import statement.
- Maintains backwards compatibility as the function signatures remain unchanged.
This refinement contributes to a more robust and maintainable codebase.
7-7: LGTM! Verify the updated import usage.The change from
TColorEditorCommandstoTEditorCommandsaligns with the type updates in thePropsinterface and suggests a refactoring to use a more generic type for editor commands. This is consistent with the PR objectives of enhancing text alignment features.To ensure the change is applied consistently, run the following script:
✅ Verification successful
Action Required: Update Verification Script to Correct Type Definitions
The previous verification script failed due to an unrecognized file type
typescriptinrg. Please use the following updated script to accurately search TypeScript files:
Verified: Import Replacement Successful
All instances of
TColorEditorCommandshave been successfully replaced withTEditorCommandsacross the codebase. The import change is consistent and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of TEditorCommands across the codebase # Test: Search for any remaining usage of TColorEditorCommands rg --type typescript 'TColorEditorCommands' # Test: Confirm the usage of TEditorCommands in relevant files rg --type typescript 'TEditorCommands'Length of output: 152
Script:
#!/bin/bash # Description: Verify the usage of TEditorCommands across the codebase # Test: Search for any remaining usage of TColorEditorCommands in .ts and .tsx files rg --glob '*.ts' --glob '*.tsx' 'TColorEditorCommands' # Test: Confirm the usage of TEditorCommands in relevant .ts and .tsx files rg --glob '*.ts' --glob '*.tsx' 'TEditorCommands'Length of output: 3685
web/core/components/pages/editor/header/toolbar.tsx (6)
6-6: LGTM: Explicit import ofEditorRefApiThe addition of this import improves type safety and makes the dependencies of this component more explicit.
95-96: LGTM: Consistent use ofitemKeyandextraPropsThe update to
isMenuItemActiveis consistent with the changes made elsewhere in the file. This improves the overall consistency of how toolbar items are handled.
116-116: LGTM: Consistent use ofrenderKeyfor React key propThe update to use
item.renderKeyas the key forCustomMenu.MenuItemis a good change. It ensures consistency with other parts of the component and follows React best practices for list rendering.
120-121: LGTM: Consistent use ofitemKeyandextraPropsThe update to
executeMenuItemCommandis consistent with the changes made elsewhere in the file. This improves the overall consistency of how toolbar items are handled and executed.
129-131: LGTM: Consistent use ofitemKeyfor active item checkThe update to use
activeTypography?.itemKeyfor checking the active item is consistent with the changes made elsewhere in the file. This improves the overall consistency of how active toolbar items are identified.
153-155: LGTM: Consistent use ofrenderKeyfor React key prop and active state trackingThe updates to use
item.renderKeyas the key forToolbarButtonand for tracking active states are good changes. They ensure consistency with other parts of the component and follow React best practices for list rendering and state management.packages/editor/src/core/components/menus/bubble-menu/root.tsx (4)
18-19: LGTM: Import of TextAlignmentSelectorThe import of
TextAlignmentSelectoraligns with the PR objectives of adding text alignment features. The import statement follows correct syntax and naming conventions.
Line range hint
136-153: LGTM: Updated rendering logic for formatting buttonsThe rendering logic for formatting buttons has been successfully updated to use
basicFormattingOptions. This change is consistent with the earlier variable renaming and maintains the existing functionality. The conditional rendering based on theisActivestate ensures proper visual feedback for active formatting options.
Line range hint
1-159: Summary: Successful integration of text alignment featuresThe changes to
EditorBubbleMenusuccessfully integrate text alignment functionality into the editor. Key improvements include:
- Addition of the
TextAlignmentSelectorcomponent.- Refactoring of formatting options logic for better code organization.
- Consistent updates to the rendering logic.
These changes align well with the PR objectives and enhance the editor's capabilities. The implementation follows React best practices and maintains good code readability.
To ensure complete functionality:
- Verify the
TextAlignmentSelectorimplementation inalignment-selector.tsx.- Confirm that all specified alignment options (Left, Center, Right) are available.
- Test the keyboard shortcuts (Cmd + Shift + L/E/R) for alignment.
Overall, this is a solid enhancement to the editor's functionality.
155-155: Approved: Addition of TextAlignmentSelectorThe inclusion of the
TextAlignmentSelectorcomponent aligns with the PR objectives of introducing text alignment features. It's correctly placed within theEditorBubbleMenustructure and receives the necessaryeditorprop.To ensure full functionality:
- Verify that the
TextAlignmentSelectorcomponent is correctly implemented in thealignment-selector.tsxfile.- Confirm that it handles the text alignment options (Left, Center, Right) as specified in the PR objectives.
- Check if the component implements the keyboard shortcuts (Cmd + Shift + L/E/R) for alignment.
packages/editor/src/core/hooks/use-editor.ts (2)
142-142: Simplified command execution logic: ApprovedThis change simplifies the
executeMenuItemCommandmethod by passing the entirepropsobject to thecommandfunction. This refactoring improves code maintainability and flexibility, making it easier to add new menu item types in the future without modifying this method. It also reduces the risk of errors that could occur with type-specific conditional checks.
Line range hint
142-155: Overall assessment: Improved menu item handlingThe changes in this file significantly improve the handling of menu items in the editor. By simplifying both the
executeMenuItemCommandandisMenuItemActivemethods, the code becomes more maintainable and flexible. This refactoring aligns well with the PR objectives of introducing text alignment features, as it makes it easier to add new menu items without modifying the core logic.These changes contribute to a more robust and extensible editor implementation, which will facilitate the integration of the new text alignment features and potential future enhancements.
packages/editor/src/styles/editor.css (1)
195-195: LGTM! Consider verifying responsive behavior.The increase in max-width for
.tippy-boxfrom 400px to 600px is a good improvement that allows for more content in tooltips or popovers. This change aligns well with the PR objectives of enhancing text alignment features.To ensure this change doesn't cause layout issues on smaller screens, please run the following verification script:
space/core/components/editor/toolbar.tsx (2)
65-97: Code logic and rendering appear correctThe rendering logic for toolbar items and the management of active states are implemented correctly. The use of
activeStatesand conditional classes ensures the toolbar reflects the current editor state accurately.
14-14: Verify prop type changes in dependent componentsThe prop types for
executeCommandandeditorRefhave been updated. Ensure that all components usingIssueCommentToolbarare passing the correct types for these props to prevent any type-related issues.Run the following script to find usages of
IssueCommentToolbarand check prop types:Also applies to: 19-19
packages/editor/src/core/types/editor.ts (1)
88-89: Enhanced type safety in method signaturesThe updated method signatures for
executeMenuItemCommandandisMenuItemActivenow utilize generic type parameters withTCommandWithPropsWithItemKey<T>. This improvement enhances type safety and ensures that the methods are used correctly with the appropriate command props.web/core/constants/editor.ts (12)
36-38: Correct Implementation ofExtraPropsForCommandUtility TypeThe
ExtraPropsForCommandtype is correctly defined using conditional types to ensure thatextraPropsare properly typed based on the command. This enhances type safety and code maintainability.
50-57: Improved Type Safety forTYPOGRAPHY_ITEMSBy specifying the generic type
ToolbarMenuItem<"text" | "h1" | "h2" | "h3" | "h4" | "h5" | "h6">, you enhance type safety for theTYPOGRAPHY_ITEMSarray, restrictingitemKeyto the specified commands. This is a good practice to prevent unintended errors.
60-94: Addition ofTEXT_ALIGNMENT_ITEMSThe introduction of
TEXT_ALIGNMENT_ITEMScorrectly extends the toolbar functionality to include text alignment options, aligning with the PR objective to add text alignment features. The integration appears consistent with existing patterns.
66-67: Ensure Consistency of Keyboard Shortcuts Across PlatformsThe shortcuts
["Cmd", "Shift", "L"],["Cmd", "Shift", "E"], and["Cmd", "Shift", "R"]are specified for text alignment. Please ensure that these shortcuts are mapped appropriately for non-Mac platforms (e.g., usingCtrlinstead ofCmd) to provide a consistent user experience.
69-92: Confirm Proper Handling ofextraPropsin Command ExecutionThe
extraPropsfield is used to pass thealignmentproperty to the command handler. Verify that the command execution logic for"text-align"correctly utilizesextraProps.alignmentto apply the desired text alignment.
96-120: Consistent Type Annotations inBASIC_MARK_ITEMSThe
BASIC_MARK_ITEMSarray now includes a generic type parameter, enhancing type safety. The items correctly specify theeditorsarray to indicate where they are applicable.
131-150: Enhanced Type Safety inLIST_ITEMSThe type annotations for
LIST_ITEMSare updated to specify the exact command keys. This improves type checking and reduces potential runtime errors.
158-160: Validation ofUSER_ACTION_ITEMSConfigurationThe
USER_ACTION_ITEMSarray is correctly updated with the generic type parameter and maintains consistency with other toolbar item definitions.
163-165: Proper Configuration ofCOMPLEX_ITEMSThe
COMPLEX_ITEMSarray now includes proper type annotations and integrates theextraPropswhere necessary.
170-182: Integration ofTEXT_ALIGNMENT_ITEMSintoTOOLBAR_ITEMSThe
TOOLBAR_ITEMSobject includes the newalignmentcategory for bothliteanddocumenteditors. Filtering based on the editor type is correctly implemented.
175-182: Verify Filtering Logic inTOOLBAR_ITEMSEnsure that the
.filtermethod accurately filters toolbar items based on theeditorsarray. This is crucial to display the correct toolbar options for each editor type.
3-5: Verify Usage of Imported IconsThe icons
AlignCenter,AlignLeft, andAlignRightare newly imported. Please ensure these icons are utilized within the codebase to avoid unnecessary imports.Run the following script to check if the imported icons are used:
✅ Verification successful
Imported icons are properly utilized across the codebase.
All imported icons (
AlignCenter,AlignLeft, andAlignRight) are actively used within the codebase, ensuring there are no unnecessary imports.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the imported icons are used in the file. # Test: Search for usages of the imported icons. for icon in "AlignCenter" "AlignLeft" "AlignRight"; do usage_count=$(rg -c "$icon" web/core/constants/editor.ts) if [ "$usage_count" -le 1 ]; then echo "Icon $icon might be unused." fi doneLength of output: 407
Script:
#!/bin/bash # Description: Verify that the imported icons are used across the entire codebase. # Test: Search for usages of the imported icons in all files except the constants file. for icon in "AlignCenter" "AlignLeft" "AlignRight"; do usage_count=$(rg -c "$icon" --glob '!web/core/constants/editor.ts' .) if [ "$usage_count" -eq 0 ]; then echo "Icon $icon might be unused across the codebase." fi doneLength of output: 2222
Script:
#!/bin/bash # Description: Verify that the imported icons are used across the entire codebase. # Test: Search for total usages of the imported icons in all files except the constants file. for icon in "AlignCenter" "AlignLeft" "AlignRight"; do usage_count=$(rg "$icon" --glob '!web/core/constants/editor.ts' -o | wc -l) if [ "$usage_count" -eq 0 ]; then echo "Icon $icon might be unused across the codebase." fi doneLength of output: 464
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (17)
packages/editor/src/core/extensions/text-align.ts (1)
1-6: Consider adding documentation to improve code maintainability.To enhance code readability and maintainability, consider adding comments or documentation to explain the purpose of this file and how to use the
CustomTextAlignExtension. This will be helpful for other developers who might work on this code in the future.Here's a suggestion for documentation:
/** * Custom Text Alignment Extension * * This file configures and exports a custom text alignment extension for the editor. * It uses the TextAlign extension from @tiptap/extension-text-align and configures * it with specific alignment options and content types. * * Usage: * Import this extension and add it to your Tiptap editor's extension array. */ import TextAlign from "@tiptap/extension-text-align"; export const CustomTextAlignExtension = TextAlign.configure({ alignments: ["left", "center", "right"], types: ["heading", "paragraph"], });packages/editor/src/core/types/slash-commands-suggestion.ts (1)
Line range hint
6-21: LGTM with a minor suggestion for consistencyThe changes to the
ISlashCommandIteminterface, specifically thecommandKeyproperty, correctly reference the newly importedTEditorCommandstype. The rest of the type definitions remain unchanged, which helps maintain compatibility with existing code.For consistency with the new import style, consider using the full import path for the React types as well. This change is optional but could improve consistency across the codebase:
-import { CSSProperties } from "react"; +import type { CSSProperties } from "react";This change emphasizes that we're importing a type and matches the style used for
TEditorCommands.packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (2)
14-17: LGTM: Component declaration and setup look good.The
TextAlignmentSelectorcomponent is correctly declared as a React functional component with proper prop typing. The use ofTextAlignItemsuggests good integration with existing functionality.Consider using object destructuring in the function parameters for cleaner code:
-export const TextAlignmentSelector: React.FC<Props> = (props) => { - const { editor } = props; +export const TextAlignmentSelector: React.FC<Props> = ({ editor }) => {
67-88: LGTM: Rendering logic is clean and efficient.The component's rendering logic is well-implemented, using React's mapping function to generate buttons for each alignment option. The use of Tailwind CSS classes and the
cnhelper function provides flexible and conditional styling.To improve accessibility, consider adding an
aria-labelto each button to describe its function:<button key={item.renderKey} type="button" + aria-label={`Align text ${item.renderKey.split('-').pop()}`} onClick={(e) => { item.command(); e.stopPropagation(); }} className={cn( // ... (existing classes) )} > <item.icon className="size-4" /> </button>space/core/components/editor/lite-text-editor.tsx (1)
70-70: LGTM. Consider movingeditorRefderivation.The simplification of passing
editorRefdirectly toIssueCommentToolbaris a good change. It removes unnecessary conditional logic and makes the code more readable.To further improve code readability and maintainability, consider moving the
editorRefderivation closer to where it's used. This would make the connection between the derivation and its usage more apparent. For example:const editorRef = isMutableRefObject<EditorRefApi>(ref) ? ref.current : null; return ( <div className="border border-custom-border-200 rounded p-3 space-y-3"> <LiteTextEditorWithRef // ... other props ... /> <IssueCommentToolbar // ... other props ... editorRef={editorRef} /> </div> );This change would group related logic together and make the component's structure easier to understand at a glance.
packages/editor/src/core/extensions/core-without-props.ts (1)
21-21: Consider additional improvements for text alignment featureThe addition of
CustomTextAlignExtensionsuccessfully introduces text alignment functionality to the editor. However, consider the following improvements:
- If not implemented elsewhere, add configuration options to
CustomTextAlignExtensionto allow for customization of alignment options or default behavior.- Ensure that the new extension doesn't conflict with existing extensions, especially those that might affect text positioning or layout.
- If keyboard shortcuts are not implemented in this extension, make sure they are properly set up in the appropriate part of the codebase to fulfill the PR objectives.
Would you like assistance in implementing any of these suggestions?
Also applies to: 91-91
packages/editor/src/core/extensions/read-only-extensions.tsx (1)
Line range hint
1-132: Summary of changes and recommendationsThe changes to this file are minimal and focused on adding the
CustomTextAlignExtension. The implementation follows existing patterns and is technically correct. However, there's a potential logical inconsistency:
- The function is named
CoreReadOnlyEditorExtensions, implying these extensions are for a read-only editor.- Adding a text alignment feature to a read-only editor seems counterintuitive, as users typically can't modify text in a read-only context.
Recommendations:
- Clarify the intended behavior of
CustomTextAlignExtensionin a read-only context.- If it's meant to display aligned text rather than allow alignment changes, consider adding a comment explaining this.
- If it's not meant for read-only use, consider creating a separate function for editable editor extensions or renaming this function to reflect its actual purpose.
These clarifications will improve code maintainability and prevent confusion for future developers working on this codebase.
web/core/components/editor/lite-text-editor/lite-text-editor.tsx (1)
69-69: LGTM! Consider using optional chaining for clarity.The addition of the
editorRefvariable improves the code by simplifying the ref handling. To further enhance readability, consider using optional chaining:const editorRef = isMutableRefObject(ref) ? ref.current : null;This change would make it clear that
ref.currentis only accessed ifrefis a mutable ref object.packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (1)
90-90: Consistent improvement for background color selectionThe modification to pass an object
{ color: color.key }instead of justcolor.keyto theBackgroundColorItem(editor).command()function is consistent with the text color change. This enhances type safety and improves API clarity.Consider refactoring the color selection logic to reduce duplication between text and background color handling. For example:
const handleColorSelection = (ColorItem: typeof TextColorItem | typeof BackgroundColorItem, color: string | undefined) => { ColorItem(editor).command(color ? { color } : undefined); }; // Usage onClick={() => handleColorSelection(TextColorItem, color.key)} onClick={() => handleColorSelection(BackgroundColorItem, color.key)}This refactoring would centralize the color selection logic and make future changes easier to manage.
web/core/components/pages/editor/header/color-dropdown.tsx (1)
12-19: LGTM! Consider extracting the type for improved readability.The type definitions for
handleColorSelectandisColorActivehave been updated to useExtract<TEditorCommands, "text-color" | "background-color">instead ofTColorEditorCommands. This change improves type safety by restricting the allowed values to "text-color" and "background-color".To improve readability, consider extracting the new type into a separate type alias:
type ColorEditorCommands = Extract<TEditorCommands, "text-color" | "background-color">; type Props = { handleColorSelect: (key: ColorEditorCommands, color: string | undefined) => void; isColorActive: (key: ColorEditorCommands, color: string | undefined) => boolean; };This extraction would make the
Propstype more concise and easier to understand at a glance.web/core/components/pages/editor/header/toolbar.tsx (2)
116-131: LGTM: Improved TYPOGRAPHY_ITEMS handling with a minor suggestionThe updates to the TYPOGRAPHY_ITEMS mapping are well-implemented and consistent with the changes made elsewhere in the file. The use of
renderKeyfor the React key anditemKeyfor functionality improves the separation of concerns.These changes align well with the PR objectives of introducing text alignment features.
Consider extracting the
onClickhandler into a separate function for improved readability:const handleTypographyItemClick = (item: TypographyItem) => { editorRef.executeMenuItemCommand({ itemKey: item.itemKey, ...item.extraProps, }); }; // Then in the JSX: onClick={() => handleTypographyItemClick(item)}This would make the JSX more concise and easier to read.
Line range hint
1-163: Overall improvements with some follow-up tasksThe changes in this file significantly improve the consistency and type safety of the toolbar implementation. The introduction of
renderKeyanditemKeyeffectively separates React-specific concerns from functionality, which is a good practice. These modifications lay a solid foundation for implementing the text alignment features described in the PR objectives.However, there are a few items that need attention:
- Address the TODOs related to toolbar homogenization.
- Resolve the type mismatches currently marked with
@ts-expect-error.- Remove any remaining commented-out debug code (e.g.,
console.logstatements).To ensure a smooth integration of the text alignment features:
- Verify that the
TOOLBAR_ITEMSconstant includes the new text alignment options.- Ensure that the
EditorRefApiincludes methods for applying and checking text alignment.- Consider adding unit tests for the new text alignment functionality in the toolbar.
Would you like assistance in creating tasks for these follow-up items?
packages/editor/src/core/helpers/editor-commands.ts (1)
184-186: Approve with suggestions: Enhance robustness and consistency ofsetTextAlignThe
setTextAlignfunction successfully implements the text alignment feature, which aligns with the PR objectives. However, consider the following improvements:
- Add error handling for invalid alignments to prevent potential runtime errors.
- Include a range parameter for consistency with other functions in this file.
- Consider implementing a toggle functionality for better user experience.
Here's a suggested implementation incorporating these improvements:
export const setTextAlign = (alignment: 'left' | 'center' | 'right' | 'justify', editor: Editor, range?: Range) => { if (range) { editor.chain().focus().deleteRange(range).setTextAlign(alignment).run(); } else if (editor.isActive({ textAlign: alignment })) { editor.chain().focus().unsetTextAlign().run(); } else { editor.chain().focus().setTextAlign(alignment).run(); } };This implementation:
- Uses a union type for
alignmentto prevent invalid inputs.- Includes an optional
rangeparameter for consistency.- Implements a toggle functionality by unsetting the alignment if it's already active.
packages/editor/src/core/hooks/use-editor.ts (3)
142-142: Approve change with suggestion for improved type safetyThe modification to pass the entire
propsobject to thecommandfunction simplifies the logic and increases flexibility, which aligns well with the PR objective of introducing text alignment features. This change allows for easier addition of new commands without modifying this method.However, to maintain type safety and prevent passing unnecessary data to commands, consider using a generic type parameter:
executeMenuItemCommand: <T extends { itemKey: TEditorCommands }>(props: T) => { const { itemKey } = props; const editorItems = getEditorMenuItems(editorRef.current); const getEditorMenuItem = (itemKey: TEditorCommands) => editorItems.find((item) => item.key === itemKey); const item = getEditorMenuItem(itemKey); if (item) { (item.command as (props: T) => void)(props); } else { console.warn(`No command found for item: ${itemKey}`); } },This approach ensures that only the necessary properties are passed to each command while maintaining flexibility.
155-155: Approve change with suggestion for improved type safetyThe modification to pass the entire
propsobject to theisActivefunction is consistent with the change inexecuteMenuItemCommand. This change increases flexibility and supports the PR objective by allowing for easier implementation of new active state checks, such as for text alignment features.To maintain type safety and consistency with the previous suggestion, consider using a generic type parameter here as well:
isMenuItemActive: <T extends { itemKey: TEditorCommands }>(props: T) => { const { itemKey } = props; const editorItems = getEditorMenuItems(editorRef.current); const getEditorMenuItem = (itemKey: TEditorCommands) => editorItems.find((item) => item.key === itemKey); const item = getEditorMenuItem(itemKey); if (!item) return false; return (item.isActive as (props: T) => boolean)(props); },This approach ensures type safety while maintaining the flexibility introduced by the change.
Line range hint
1-285: Summary: Changes support PR objectives with room for type safety improvementsThe modifications to
executeMenuItemCommandandisMenuItemActivemethods effectively support the PR objective of introducing text alignment features. By passing the entirepropsobject, the changes increase the flexibility and extensibility of the editor's command system, allowing for easy addition of new features without modifying these core methods.The changes align well with the implementation of text alignment features, as they can now be easily integrated into the existing command structure. This approach will facilitate the addition of toolbar buttons and keyboard shortcuts for left, center, and right alignment as specified in the PR objectives.
To further improve the implementation:
- Consider the suggested type safety enhancements to maintain robustness while preserving flexibility.
- Ensure that the new text alignment commands (
Cmd + Shift + L,Cmd + Shift + E,Cmd + Shift + R) are properly integrated into the editor's command system.- Verify that the toolbar for text alignment options is correctly implemented and interacts with these modified methods as expected.
As you proceed with the implementation of text alignment features, consider the following architectural advice:
- Implement a consistent approach for adding new commands and active state checks across the codebase.
- Document the new command structure and how to extend it for future developers.
- Consider creating unit tests for the new text alignment functionality to ensure reliability and prevent regressions.
web/core/components/editor/lite-text-editor/toolbar.tsx (1)
71-71: Address the TODO comment regarding toolbar homogenizationThere's a TODO comment indicating a pending update during toolbar homogenization. It's important to resolve this to maintain consistency across toolbars.
Would you like assistance in updating this code segment to complete the toolbar homogenization?
📜 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 (25)
- packages/editor/package.json (1 hunks)
- packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1 hunks)
- packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (2 hunks)
- packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (2 hunks)
- packages/editor/src/core/components/menus/bubble-menu/root.tsx (4 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 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/text-align.ts (1 hunks)
- packages/editor/src/core/helpers/editor-commands.ts (1 hunks)
- packages/editor/src/core/hooks/use-editor.ts (2 hunks)
- packages/editor/src/core/types/editor.ts (2 hunks)
- packages/editor/src/core/types/slash-commands-suggestion.ts (1 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- space/core/components/editor/lite-text-editor.tsx (3 hunks)
- space/core/components/editor/toolbar.tsx (3 hunks)
- space/core/components/issues/peek-overview/comment/add-comment.tsx (1 hunks)
- space/core/constants/editor.ts (3 hunks)
- web/core/components/editor/lite-text-editor/lite-text-editor.tsx (3 hunks)
- web/core/components/editor/lite-text-editor/toolbar.tsx (3 hunks)
- web/core/components/pages/editor/header/color-dropdown.tsx (1 hunks)
- web/core/components/pages/editor/header/toolbar.tsx (6 hunks)
- web/core/constants/editor.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- space/core/components/issues/peek-overview/comment/add-comment.tsx
🧰 Additional context used
🪛 Biome
packages/editor/src/core/types/editor.ts
[error] 60-60: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (69)
packages/editor/src/core/extensions/text-align.ts (3)
1-6: LGTM! The text alignment extension is correctly implemented.The
CustomTextAlignExtensionis properly configured with the required alignment options ("left", "center", "right") and applicable content types ("heading", "paragraph"). This implementation aligns well with the PR objectives for introducing text alignment features.
1-6: Verify the integration of this extension with the editor.While this file correctly exports the
CustomTextAlignExtension, it's important to ensure that it's properly integrated with the editor and works with theTextAlignmentSelectorcomponent mentioned in the AI summary.Let's verify the usage of this extension:
#!/bin/bash # Description: Verify the integration of CustomTextAlignExtension with the editor # Search for imports of CustomTextAlignExtension rg --type typescript --type javascript "import.*CustomTextAlignExtension" # Search for usage of CustomTextAlignExtension in editor configuration rg --type typescript --type javascript "CustomTextAlignExtension" -C 5Please ensure that the
CustomTextAlignExtensionis properly added to the editor's extension array and that it interacts correctly with theTextAlignmentSelectorcomponent.
1-6: Clarify the implementation of keyboard shortcuts.The PR objectives mention keyboard shortcuts for text alignment (Cmd + Shift + L, Cmd + Shift + E, Cmd + Shift + R), but these are not visible in this implementation. Could you clarify where and how these shortcuts are being handled?
To verify the implementation of keyboard shortcuts, we can search for their usage in the codebase:
If the keyboard shortcuts are not yet implemented, would you like assistance in adding them to the appropriate component or handler?
packages/editor/src/core/types/slash-commands-suggestion.ts (1)
3-4: Verify the impact of centralizingTEditorCommandsThe
TEditorCommandstype is now imported from a central location (@/types) instead of being defined in this file. While this change can improve maintainability, it's important to ensure that all other parts of the codebase that may have been importingTEditorCommandsfrom this file have been updated accordingly.Additionally, the removal of
TColorEditorCommandsandTNonColorEditorCommandstypes suggests a change in how editor commands are categorized or used. Please confirm that this removal doesn't negatively impact any existing functionality.To ensure all usages have been updated, run the following script:
If these searches return results, those locations may need to be updated to reflect the new import location or to remove usage of the deleted types.
✅ Verification successful
Reattempting search with corrected file type for TypeScript
The previous search attempts failed because
typescriptis not a recognized file type inrg. Let's retry the searches using the correct file type or by specifying the file extensions directly.
Reattempting search using file extension patterns for TypeScript
The previous search attempts failed due to incorrect file type specifications. Let's retry the searches by directly specifying the
.tsand.tsxfile extensions to ensure accurate results.
Verification Successful: No residual imports or usages found
The searches for any remaining imports of
TEditorCommandsfromslash-commands-suggestion.tsand usages ofTColorEditorCommandsorTNonColorEditorCommandsreturned no results. This indicates that the centralization and removal of these types have been properly handled across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining imports of TEditorCommands from this file # Test: Search for imports of TEditorCommands from slash-commands-suggestion rg --type typescript "import.*TEditorCommands.*from.*slash-commands-suggestion" # Test: Search for uses of TColorEditorCommands and TNonColorEditorCommands rg --type typescript "TColorEditorCommands|TNonColorEditorCommands"Length of output: 221
Script:
#!/bin/bash # Description: Check for any remaining imports of TEditorCommands from this file using the correct file type # Test: Search for imports of TEditorCommands from slash-commands-suggestion rg --type ts "import.*TEditorCommands.*from.*slash-commands-suggestion" # Test: Search for uses of TColorEditorCommands and TNonColorEditorCommands rg --type ts "TColorEditorCommands|TNonColorEditorCommands"Length of output: 135
Script:
#!/bin/bash # Description: Check for any remaining imports of TEditorCommands from slash-commands-suggestion using file extension patterns # Test: Search for imports of TEditorCommands from slash-commands-suggestion in .ts and .tsx files rg "import.*TEditorCommands.*from.*slash-commands-suggestion" --glob "*.ts" --glob "*.tsx" # Test: Search for uses of TColorEditorCommands and TNonColorEditorCommands in .ts and .tsx files rg "TColorEditorCommands|TNonColorEditorCommands" --glob "*.ts" --glob "*.tsx"Length of output: 173
packages/editor/src/core/extensions/index.ts (3)
19-19: LGTM: Addition of headers exportThe addition of
export * from "./headers"aligns with the PR objective of introducing text alignment features. Headers are often used in text editors and may be related to text alignment functionality.
25-25: LGTM: Addition of text-align exportThe addition of
export * from "./text-align"directly relates to the PR objective of adding text alignment features. This module likely contains the core functionality for text alignment, which is a key component of this PR.
Line range hint
1-26: Verify discrepancies with AI summaryThe AI-generated summary mentions the removal of exports for "./slash-commands" and "./headers", but these changes are not visible in the provided code. This could be due to limitations in the diff view or multiple commits in the PR.
To verify these discrepancies, please run the following script:
This script will help us understand if the mentioned removals actually occurred and if there were any intermediate changes not reflected in the final diff.
packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (3)
1-12: LGTM: Imports and type definition are appropriate.The imports cover all necessary dependencies for the component, including the Tiptap
Editor, icon components, and custom helpers. ThePropstype is correctly defined with theeditorprop of typeEditor.
1-89: Great implementation of text alignment feature.The
TextAlignmentSelectorcomponent successfully implements the text alignment feature as described in the PR objectives. It provides options for left, center, and right alignment, which can be accessed through the editor's toolbar.Key points:
- The component integrates well with the Tiptap editor, ensuring compatibility with keyboard shortcuts.
- The implementation is clean, efficient, and follows React best practices.
- The use of Tailwind CSS provides flexible and maintainable styling.
To ensure full compatibility with the PR objectives, please confirm that the keyboard shortcuts (
Cmd + Shift + L,Cmd + Shift + E,Cmd + Shift + R) are properly set up in the Tiptap editor configuration. You can verify this by running the following script:If the shortcuts are not found, consider adding them to the Tiptap editor configuration to fully meet the PR objectives.
19-65: LGTM: Text alignment options are well-defined.The
textAlignmentOptionsarray is structured correctly, covering left, center, and right alignments. Each option includes the necessary properties for rendering and functionality.Could you clarify the purpose of the
itemKeyandrenderKeyproperties? They seem potentially redundant, asitemKeyis always "text-align" andrenderKeyappears to be a unique identifier. If they're used elsewhere in the application, please provide context. Otherwise, consider simplifying the option structure.To verify the usage of these properties, you can run the following script:
space/core/components/editor/lite-text-editor.tsx (3)
36-37: LGTM. Good use of derived state.The addition of the
isEmptyvariable is a good practice. It improves code readability by giving a clear name to the condition and allows for easy reuse of this derived state.This change enhances the maintainability of the component by making the empty state check more explicit and easier to understand.
Line range hint
1-77: Overall assessment: Changes align with PR objectives, with minor improvements needed.The modifications to the
LiteTextEditorcomponent align well with the PR objectives of introducing text alignment features across various editors. The changes to the editor commands and toolbar suggest progress towards implementing the text alignment functionality.Key points:
- The updated import statements and command execution logic support the new text alignment feature.
- The simplification of
editorRefhandling improves code readability.- The addition of the
isEmptyvariable enhances maintainability.To fully meet the PR objectives, ensure that:
- The text alignment options (Left, Center, Right) are properly implemented in the
IssueCommentToolbar.- The keyboard shortcuts (Cmd + Shift + L/E/R) for text alignment are correctly set up.
- The text alignment feature works across all specified editors (pages, issue descriptions, and comments).
To verify the implementation of text alignment features, please run the following script:
#!/bin/bash # Description: Check for text alignment implementation # Test 1: Search for text alignment related imports echo "Checking for text alignment imports:" rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js '@tiptap/extension-text-align' # Test 2: Search for text alignment command handlers echo "Checking for text alignment command handlers:" rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js 'textAlign|alignLeft|alignCenter|alignRight' # Test 3: Search for keyboard shortcut definitions echo "Checking for text alignment keyboard shortcuts:" rg --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' --type ts --type js 'Cmd \+ Shift \+ [LER]|Command \+ Shift \+ [LER]'This script will help verify that the text alignment feature is properly implemented across the codebase.
3-3: LGTM. Verify impact on editor commands.The import statement has been updated to include necessary types and components. The removal of
TNonColorEditorCommandssuggests a change in how editor commands are handled.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining usage of
TNonColorEditorCommands:packages/editor/package.json (1)
51-51: LGTM! Verify usage and version.The addition of
"@tiptap/extension-text-align": "^2.8.0"aligns with the PR objectives of introducing text alignment features. This is a positive change that will enable the implementation of text alignment functionality.To ensure this dependency is properly utilized, please run the following verification script:
This script will help confirm that the new dependency is being used in the codebase and whether the specified version (^2.8.0) is the latest stable version. If the usage is not found or if there's a newer stable version available, consider updating accordingly.
packages/editor/src/core/extensions/core-without-props.ts (3)
21-21: LGTM: Import statement for CustomTextAlignExtensionThe import statement for
CustomTextAlignExtensionis correctly added and follows the existing pattern in the file. This aligns with the PR objective of introducing text alignment features.
91-91: LGTM: Addition of CustomTextAlignExtensionThe
CustomTextAlignExtensionis correctly added to theCoreEditorExtensionsWithoutPropsarray. Its placement at the end of the array is appropriate and doesn't interfere with existing extensions.
91-91: Verify implementation of keyboard shortcutsThe PR objectives mention keyboard shortcuts for text alignment (Cmd + Shift + L, Cmd + Shift + E, Cmd + Shift + R), but there's no visible implementation of these shortcuts in this file. Please ensure that these shortcuts are implemented elsewhere in the codebase.
packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (4)
22-23: LGTM: Import ofTEditorCommandstypeThe addition of the
TEditorCommandstype import enhances type safety, which aligns with good TypeScript practices and the PR's objectives.
34-34: LGTM: Enhanced type safety foritemsarrayThe update to
EditorMenuItem<TEditorCommands>[]improves type safety by ensuring that the commands associated with each menu item are explicitly typed. This change aligns well with TypeScript best practices and the PR's objectives.
Line range hint
1-99: Summary: Type safety improvements implemented, but text alignment features not found in this fileThe changes in this file enhance type safety, which is a positive improvement. However, the main objectives of the PR, specifically the text alignment features (Left, Center, Right) and their corresponding keyboard shortcuts, are not implemented in this file.
Could you please clarify:
- In which file(s) are the text alignment features implemented?
- How do these changes in the
BubbleMenuNodeSelectorcomponent relate to the text alignment functionality?- Are there any plans to add text alignment options to this menu, or is it intentionally separate?
To help locate the text alignment implementations, I've provided a script in a previous comment. Running this script should help identify where these features are implemented across the codebase.
Line range hint
34-46: Verify implementation of text alignment optionsThe PR objectives mention adding text alignment features (Left, Center, Right) to various editors. However, I don't see these options in the
itemsarray. Could you please verify if the text alignment options are implemented elsewhere or if they need to be added to this component?To check for text alignment implementations, we can run the following script:
packages/editor/src/core/extensions/read-only-extensions.tsx (2)
26-26: LGTM: Import statement for CustomTextAlignExtensionThe import statement for
CustomTextAlignExtensionis correctly added and follows the existing pattern for importing custom extensions.
130-130: Verify the necessity of CustomTextAlignExtension in a read-only contextThe
CustomTextAlignExtensionhas been correctly added to the array of extensions. However, I have a concern:This function is named
CoreReadOnlyEditorExtensions, which implies these extensions are for a read-only editor. Adding a text alignment feature to a read-only editor seems counterintuitive, as users typically can't modify text in a read-only context.Could you please clarify the intended behavior? If this extension is meant to display aligned text in read-only mode (rather than allow users to change alignment), it might be worth adding a comment explaining this to prevent confusion for future developers.
To verify the intended behavior, we can check if there's a read-only version of the
CustomTextAlignExtension:web/core/components/editor/lite-text-editor/lite-text-editor.tsx (2)
67-68: LGTM! Improved variable positioning.The
isEmptyvariable has been correctly moved inside the component function. This ensures that it's recalculated on each render, which is appropriate for a derived value based on props.
3-3: LGTM! Consider verifying unused imports.The import statement has been updated to include
EditorRefApiand removeTNonColorEditorCommands. This change aligns with the modifications in the component.To ensure all imports are being used and no unnecessary imports remain, run the following command:
✅ Verification successful
LGTM! Unused imports verified.
All imports in
web/core/components/editor/lite-text-editor/lite-text-editor.tsxare used. No unnecessary imports found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Look for imports that are not used in the file rg --type typescript -e '^import.*from' web/core/components/editor/lite-text-editor/lite-text-editor.tsx | while read -r line; do import=$(echo "$line" | sed -E 's/^import \{([^}]*)\}.*/\1/' | tr ',' '\n' | sed 's/^ *//; s/ *$//') echo "$import" | while read -r item; do if ! rg -q "\b$item\b" web/core/components/editor/lite-text-editor/lite-text-editor.tsx; then echo "Potentially unused import: $item" fi done doneLength of output: 156
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Look for import statements and verify their usage rg -e '^import\s+\{[^}]+\}\s+from' web/core/components/editor/lite-text-editor/lite-text-editor.tsx | while read -r line; do # Extract imported items imports=$(echo "$line" | sed -E 's/^import\s+\{([^}]+)\}\s+from.*/\1/' | tr ',' '\n' | sed 's/^ *//; s/ *$//') for import in $imports; do # Check if the import is used elsewhere in the file if ! rg -q "\\b$import\\b" web/core/components/editor/lite-text-editor/lite-text-editor.tsx; then echo "Potentially unused import: $import" fi done doneLength of output: 3510
packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (2)
67-67: Improved type safety for text color selectionThe modification to pass an object
{ color: color.key }instead of justcolor.keyto theTextColorItem(editor).command()function enhances type safety and makes the API more explicit. This change aligns well with the PR objectives and improves the overall code quality.
Line range hint
1-106: Overall implementation looks goodThe changes to the
BubbleMenuColorSelectorcomponent improve type safety and align well with the PR objectives for implementing text alignment features. The modifications are consistent between text and background color handling.To ensure the changes are working as expected, please verify the following:
- Test the text and background color selection functionality in the editor.
- Confirm that the color changes are applied correctly when selected.
- Check if the color selection integrates well with the new text alignment features.
This script will help identify any potential integration points between color selection and text alignment features.
✅ Verification successful
Verification Successful
The
TextColorItemandBackgroundColorItemare correctly integrated with the text alignment features within the codebase. TheBubbleMenuColorSelectorcomponent interacts seamlessly with the alignment functionalities, aligning with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the integration of color selection with text alignment # Search for usage of TextColorItem and BackgroundColorItem echo "Searching for TextColorItem and BackgroundColorItem usage:" rg --type typescript "TextColorItem|BackgroundColorItem" # Search for text alignment related code echo "\nSearching for text alignment related code:" rg --type typescript "textAlign|alignLeft|alignCenter|alignRight"Length of output: 422
Script:
#!/bin/bash # Verify the integration of color selection with text alignment # Search for usage of TextColorItem and BackgroundColorItem echo "Searching for TextColorItem and BackgroundColorItem usage:" rg "TextColorItem|BackgroundColorItem" --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx # Search for text alignment related code echo "\nSearching for text alignment related code:" rg "textAlign|alignLeft|alignCenter|alignRight" --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsxLength of output: 550
Script:
#!/bin/bash # Verify the integration of color selection with text alignment # Search for usage of TextColorItem and BackgroundColorItem echo "Searching for TextColorItem and BackgroundColorItem usage:" rg "TextColorItem|BackgroundColorItem" -g "*.ts" -g "*.tsx" # Search for text alignment related code echo "\nSearching for text alignment related code:" rg "textAlign|alignLeft|alignCenter|alignRight" -g "*.ts" -g "*.tsx"Length of output: 1941
Script:
#!/bin/bash # Verify the integration of color selection with text alignment # Search for usage of TextColorItem and BackgroundColorItem in .ts and .tsx files echo "Searching for TextColorItem and BackgroundColorItem usage:" rg "TextColorItem|BackgroundColorItem" -g "*.ts*" # Search for text alignment related code in .ts and .tsx files echo "\nSearching for text alignment related code:" rg "textAlign|alignLeft|alignCenter|alignRight" -g "*.ts*"Length of output: 1921
packages/editor/src/core/extensions/extensions.tsx (3)
21-21: LGTM: Import of CustomTextAlignExtensionThe addition of
CustomTextAlignExtensionto the import statement is consistent with the existing code structure and aligns with the PR objective of introducing text alignment features.
21-21: Verify toolbar and keyboard shortcut implementationThe changes in this file successfully introduce the text alignment extension. However, the PR objectives mention a toolbar and keyboard shortcuts for text alignment, which are not directly addressed here.
Please ensure that the toolbar and keyboard shortcuts for text alignment (Cmd + Shift + L for left, Cmd + Shift + E for center, and Cmd + Shift + R for right) are implemented in the appropriate components. You can verify this with the following script:
#!/bin/bash # Description: Verify toolbar and keyboard shortcut implementation for text alignment # Test: Check for toolbar implementation rg --type typescript 'TextAlignmentSelector|alignLeft|alignCenter|alignRight' -A 5 -B 5 # Test: Check for keyboard shortcut implementation rg --type typescript 'Cmd \+ Shift \+ [LER]|CommandShift[LER]' -A 5 -B 5Also applies to: 161-161
161-161: LGTM: Integration of CustomTextAlignExtensionThe addition of
CustomTextAlignExtensionto the array returned byCoreEditorExtensionsis correct and aligns with the PR objective of introducing text alignment features.To ensure proper integration, let's verify the implementation of
CustomTextAlignExtension:✅ Verification successful
LGTM: Integration of CustomTextAlignExtension
The implementation of
CustomTextAlignExtensionintext-align.tscorrectly includes the alignment options["left", "center", "right"], ensuring the desired text alignment functionality is available in the editor.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of CustomTextAlignExtension # Test: Check if CustomTextAlignExtension is properly implemented ast-grep --lang typescript --pattern 'export const CustomTextAlignExtension = $_' # Test: Verify if the extension includes left, center, and right alignment options rg --type typescript 'alignLeft|alignCenter|alignRight' -A 5 -B 5Length of output: 328
Script:
#!/bin/bash # Description: Verify the implementation of CustomTextAlignExtension # Test: Check if CustomTextAlignExtension is properly implemented ast-grep --lang typescript --pattern 'export const CustomTextAlignExtension = $_' # Test: Verify if the extension includes left, center, and right alignment options rg --type ts 'alignLeft|alignCenter|alignRight' -A 5 -B 5 rg --type tsx 'alignLeft|alignCenter|alignRight' -A 5 -B 5Length of output: 584
Script:
#!/bin/bash # Description: Verify the implementation of CustomTextAlignExtension by searching alignment options in .ts and .tsx files # Test: Check if CustomTextAlignExtension is properly implemented ast-grep --lang typescript --pattern 'export const CustomTextAlignExtension = $_' # Test: Verify if the extension includes left, center, and right alignment options in .ts files rg 'alignLeft|alignCenter|alignRight' -A 5 -B 5 -g "*.ts" # Test: Verify if the extension includes left, center, and right alignment options in .tsx files rg 'alignLeft|alignCenter|alignRight' -A 5 -B 5 -g "*.tsx"Length of output: 556
web/core/components/pages/editor/header/color-dropdown.tsx (2)
Line range hint
1-138: Overall assessment: Changes improve type safety and align with PR objectives.The modifications to this file focus on enhancing type safety and consistency in the
ColorDropdowncomponent. The changes align well with the PR objectives by refining the type definitions related to text color and background color. These improvements contribute to the overall goal of enhancing the editor's functionality and type safety.Key points:
- The import statement has been updated to use a more generic
TEditorCommandstype.- The
Propstype definition now uses a more specific type for color-related commands.- No functional changes were made to the component's behavior, maintaining its existing functionality.
These changes are part of the larger effort to implement text alignment features across various editors, as mentioned in the PR objectives.
7-7: LGTM! Verify the impact of the import change.The import statement has been updated to use
TEditorCommandsinstead ofTColorEditorCommands. This change aligns with the modifications in thePropstype and suggests a move towards using a more generic type.To ensure this change doesn't have unintended consequences, please run the following script:
If the script returns any results, those occurrences might need to be updated as well.
✅ Verification successful
Import Change Verified: No remaining usage of
TColorEditorCommandsfound.The update to use
TEditorCommandsis safe and does not affect other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of TColorEditorCommands in the codebase # Test: Search for TColorEditorCommands usage rg --type typescript 'TColorEditorCommands'Length of output: 78
Script:
#!/bin/bash # Description: Check for any remaining usage of TColorEditorCommands in TypeScript files # Test: Search for TColorEditorCommands usage in .ts and .tsx files rg 'TColorEditorCommands' --type ts --type tsxLength of output: 74
Script:
#!/bin/bash # Description: Check for any remaining usage of TColorEditorCommands in .ts and .tsx files # Test: Search for TColorEditorCommands usage in .ts and .tsx files using glob patterns rg 'TColorEditorCommands' --glob '*.ts' --glob '*.tsx'Length of output: 54
web/core/components/pages/editor/header/toolbar.tsx (4)
6-6: LGTM: Improved type safety with EditorRefApi importThe addition of the
EditorRefApiimport from "@plane/editor" enhances type safety for editor-related operations. This change aligns well with the goal of improving overall type safety in the codebase.
95-96: LGTM: Consistent usage of itemKey and extraPropsThe update to the
activeTypographyvariable, now usingitem.itemKeyandextraProps, aligns well with the changes made elsewhere in the file. This improves consistency in how toolbar items are handled throughout the component.
153-155: LGTM: Consistent ToolbarButton renderingThe updates to the ToolbarButton rendering are well-implemented and consistent with the changes made elsewhere in the file. The use of
item.renderKeyas the key and inactiveStatesimproves overall consistency in how toolbar items are handled.
71-84:⚠️ Potential issueClean up updateActiveStates function
The changes to
updateActiveStatesfunction improve consistency with the ToolbarButton component. However, there are several issues that need attention:
The TODO comment suggests that further updates are needed for toolbar homogenization. This aligns with the previous TODO comment.
The use of
@ts-expect-errorindicates a type mismatch that should be resolved.Commented out
console.logstatements should be removed to keep the code clean.To address these issues:
- Ensure that the follow-up task for toolbar homogenization includes this function.
- Investigate and fix the type mismatch, removing the need for
@ts-expect-error.- Remove the commented out
console.logstatements.Run the following script to check for any remaining
console.logstatements:✅ Verification successful
Remove Commented-Out
console.logStatementsThe
toolbar.tsxfile still contains commented-outconsole.logstatements:
- Lines 71-84:
// console.log("Updating status"); // console.log("newActiveStates", newActiveStates);These should be removed to maintain clean and readable code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for console.log statements in the file rg 'console\.log' web/core/components/pages/editor/header/toolbar.tsxLength of output: 166
packages/editor/src/core/components/menus/bubble-menu/root.tsx (5)
18-19: LGTM: TextAlignmentSelector import added.The import of the new
TextAlignmentSelectorcomponent is correctly placed and aligns with the PR objective of adding text alignment features to the editor.
Line range hint
136-153: LGTM: Rendering logic updated consistently.The rendering logic has been updated to use 'basicFormattingOptions' instead of 'items', which is consistent with the earlier variable renaming. The mapping logic remains unchanged, preserving the existing functionality.
Line range hint
1-160: Summary: Text alignment feature successfully integrated.The changes in this file successfully integrate the text alignment feature into the editor's bubble menu. The code modifications, including the import of TextAlignmentSelector, variable renaming, and rendering logic updates, are well-implemented and align with the PR objectives.
Some points to consider:
- Verify that the renaming of 'items' to 'basicFormattingOptions' doesn't affect other parts of the codebase.
- Ensure that the TextAlignmentSelector component correctly implements the Left, Center, and Right alignment options as specified in the PR objectives.
Overall, the changes enhance the editor's functionality while maintaining code quality and readability.
155-155: LGTM: TextAlignmentSelector added as per PR objective.The TextAlignmentSelector component has been added, which aligns with the PR objective of introducing text alignment features. Its placement after the formatting buttons is logical.
To ensure the TextAlignmentSelector is implemented correctly, please run the following script:
#!/bin/bash # Description: Verify the implementation of TextAlignmentSelector # Test: Check the implementation of TextAlignmentSelector cat packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx # Test: Verify if the component handles the alignment options (Left, Center, Right) as mentioned in the PR objectives rg --type typescript --type tsx -i '(left|center|right)\s*alignment' packages/editor
30-32: Approve renaming and conditional logic, but verify usage.The renaming of 'items' to 'basicFormattingOptions' improves code readability. The added conditional logic for including CodeItem is a good practice. However, ensure that this change doesn't break any existing functionality that might have been using the 'items' variable.
Run the following script to verify the variable usage across the codebase:
✅ Verification successful
Correct the file type in the search script and rerun the verification.
The previous script failed because
typescriptis not a recognized file type inrg. Please usetsandtsxinstead to search for any remaining usage of theitemsvariable.
Adjust the search to use file patterns instead of types and rerun the verification.
The previous script failed because
rgdoes not recognizetsxas a valid file type. Please modify the search to use file patterns*.tsand*.tsxinstead of the--typeflag.
Renaming and conditional logic verified; no remaining 'items' usage found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of 'items' variable in editor-related files # Test: Search for 'items' usage in editor-related files rg --type typescript --type tsx 'items\s*=' packages/editorLength of output: 2366
Script:
#!/bin/bash # Description: Check for any remaining usage of 'items' variable in editor-related files # Test: Search for 'items' usage in TypeScript and TSX files within packages/editor rg --type ts --type tsx 'items\s*=' packages/editorLength of output: 81
Script:
#!/bin/bash # Description: Check for any remaining usage of 'items' variable in TypeScript and TSX files within packages/editor # Test: Search for 'items' usage in .ts and .tsx files within packages/editor rg 'items\s*=' packages/editor -g '*.ts' -g '*.tsx'Length of output: 53
packages/editor/src/core/helpers/editor-commands.ts (1)
183-186: Integration ofsetTextAlignis consistent with existing codeThe placement and structure of the new
setTextAlignfunction at the end of the file are consistent with the existing code patterns. It doesn't introduce any conflicts or issues with the existing functions.packages/editor/src/styles/editor.css (1)
195-195: LGTM! Consider verifying layout on smaller screens.The increase in max-width for
.tippy-boxfrom 400px to 600px allows for more spacious tooltips or popovers, which can enhance the display of text alignment options and other editor features. This change aligns well with the PR objectives.To ensure this change doesn't cause any layout issues, especially on smaller screens, please run the following verification script:
This script will help identify any potential conflicts or areas that might need adjustment due to the increased width.
packages/editor/src/core/types/editor.ts (3)
17-40: Addition of 'text-align' command is appropriateThe inclusion of
"text-align"inTEditorCommandsaligns with the PR objectives to introduce text alignment features across editors.
42-55: 'text-align' extra properties are correctly definedDefining extra properties for
"text-align"inTCommandExtraPropswith analignmentparameter is appropriate for handling text alignment commands.
88-89: Verify compatibility of updated method signatures with existing implementationsThe methods
executeMenuItemCommandandisMenuItemActivenow use the generic type parameters withTCommandWithPropsWithItemKey<T>. Ensure all implementations of theEditorRefApiinterface are updated to match these new method signatures to prevent type errors.Run the following script to find usages of
EditorRefApiand verify method implementations:✅ Verification successful
Adjusting search commands to include
.tsand.tsxfiles correctly. Initiating re-verification.
Method signatures are compatible across all implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all implementations of EditorRefApi have updated method signatures. # Search for implementations of EditorRefApi rg --type ts --type tsx 'implements\s+EditorRefApi' # Search for definitions or overrides of executeMenuItemCommand rg --type ts --type tsx 'executeMenuItemCommand\s*[:(]' # Search for definitions or overrides of isMenuItemActive rg --type ts --type tsx 'isMenuItemActive\s*[:(]'Length of output: 247
Script:
#!/bin/bash # Description: Verify that all implementations of EditorRefApi have updated method signatures. # Search for implementations of EditorRefApi in .ts and .tsx files rg 'implements\s+EditorRefApi' -g '*.ts' -g '*.tsx' # Search for definitions or overrides of executeMenuItemCommand in .ts and .tsx files rg 'executeMenuItemCommand\s*[:(]' -g '*.ts' -g '*.tsx' # Search for definitions or overrides of isMenuItemActive in .ts and .tsx files rg 'isMenuItemActive\s*[:(]' -g '*.ts' -g '*.tsx'Length of output: 1561
space/core/constants/editor.ts (2)
35-42: Type definitions updated appropriately.The
ToolbarMenuItemtype has been enhanced with a generic parameter, allowing for extra properties specific to commands. This increases flexibility and type safety.
55-89: Text alignment items are well-defined.The
TEXT_ALIGNMENT_ITEMSconstant is correctly introduced with appropriate properties, includingitemKey,renderKey,name,icon,shortcut,editors, andextraProps. The implementation aligns with the intended functionality.web/core/components/editor/lite-text-editor/toolbar.tsx (6)
6-6: ImportingEditorRefApiThe import of
EditorRefApifrom@plane/editoraligns with the updated usage ofeditorRef.
10-10: ImportingToolbarMenuItemIncluding
ToolbarMenuItemin the imports is appropriate for handling toolbar items with improved type safety.
17-17: UpdateexecuteCommandto acceptToolbarMenuItemChanging the
executeCommandfunction to accept aToolbarMenuItemenhances flexibility and aligns with how toolbar commands are executed.
24-24: SimplifyeditorReftypeUpdating
editorRefto be of typeEditorRefApi | nullsimplifies the reference and removes unnecessary indirection.
83-84: Ensure proper subscription to editor state changesThe updated null check for
editorRefand the direct use ofeditorRef.onStateChangeimprove readability and ensure that the component correctly subscribes to editor state changes.
127-158: Enhance toolbar item rendering and active state managementUpdating the rendering logic to use
item.renderKeyand passing the entireitemtoexecuteCommandimproves type safety and consistency. The use ofactiveStates[item.renderKey]accurately reflects the active state of each toolbar item.packages/editor/src/core/components/menus/menu-items.ts (4)
23-30: New Imports for Text Alignment FeatureThe addition of
AlignCenterandsetTextAlignimports correctly integrates the text alignment functionality into the editor.
52-61: Enhanced Type Safety with Generic FunctionsExcellent work introducing the generic types
isActiveFunctionandcommandFunctionand updatingEditorMenuItemaccordingly. This improves type safety and code maintainability.
199-205: Consistent Parameter Usage inImageItemUpdating
ImageItemto accept parameters in thecommandfunction increases consistency across menu items and accommodates necessary parameters.
255-255:TextAlignItemAdded to Editor Menu ItemsIncluding
TextAlignItemingetEditorMenuItemsensures that text alignment options are available to users.web/core/constants/editor.ts (11)
3-5: Imports for text alignment icons are correctly addedThe imports of
AlignCenter,AlignLeft, andAlignRightfromlucide-reactare appropriate for the new text alignment feature.
35-38: Utility typeExtraPropsForCommandis well-definedThe
ExtraPropsForCommandtype correctly ensures thatextraPropsare enforced or made optional based on the command's requirements. This improves type safety when extending commands with additional properties.
40-47: EnhancedToolbarMenuItemtype with a generic parameterIntroducing a generic parameter
T extends TEditorCommandsin theToolbarMenuItemtype increases flexibility and type safety. It allows for specifying extra properties based on the command type.
50-57: UpdatedTYPOGRAPHY_ITEMSto utilize the genericToolbarMenuItemtypeThe
TYPOGRAPHY_ITEMSarray now correctly uses the updatedToolbarMenuItemtype with specificitemKeyandrenderKeyproperties. This change enhances type safety and aligns with the new structure.
60-94: Addition ofTEXT_ALIGNMENT_ITEMSfor text alignment commandsThe new
TEXT_ALIGNMENT_ITEMSarray introduces items for left, center, and right text alignment. Each item includes appropriate properties such asitemKey,renderKey,name,icon,shortcut,editors, andextraPropswith thealignmentvalue.
95-120: Consistent use ofrenderKeyinBASIC_MARK_ITEMSThe
BASIC_MARK_ITEMSarray now includes therenderKeyproperty for each item, ensuring consistency with other toolbar item definitions.
131-150: UpdatedLIST_ITEMSwith specificitemKeyandrenderKeypropertiesThe
LIST_ITEMSarray correctly utilizes the updatedToolbarMenuItemtype, enhancing type safety and consistency across the toolbar configurations.
158-160: UpdatedUSER_ACTION_ITEMSto use the newToolbarMenuItemtypeThe
USER_ACTION_ITEMSarray has been updated to align with the newToolbarMenuItemtype definition, promoting consistency and improved type enforcement.
163-165: UpdatedCOMPLEX_ITEMSwith enhanced type definitionsThe
COMPLEX_ITEMSarray reflects the updated type definitions, correctly specifyingitemKey,renderKey, and other properties in line with the new structure.
170-182: AdjustedTOOLBAR_ITEMSto accommodate new type definitionsThe
TOOLBAR_ITEMSexport has been updated to use the enhancedToolbarMenuItemtype. The filtering logic based on editor types remains intact, and the inclusion of the new alignment items is correctly handled.
60-94: Verify thatTCommandExtraPropsincludes thetext-aligncommand withalignmentpropertyTo ensure type safety for the
extraPropsinTEXT_ALIGNMENT_ITEMS, please confirm that theTCommandExtraPropstype includes an entry for thetext-aligncommand specifying thealignmentproperty.You can run the following script to verify:
This script searches for the
TCommandExtraPropstype definition and checks if it includes a definition for'text-align'with the appropriate properties.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
Line range hint
7-14: LGTM: Type definition enhanced for better type safety.The update to
CustomImageNodeViewPropsis a good improvement:
- It now extends
NodeViewProps, incorporating all relevant properties from Tiptap's node view.- The additional properties are correctly typed and consistent with the component's usage.
- This change supports potential new features, aligning with the PR objectives.
Consider adding a comment explaining the purpose of extending
NodeViewPropsand any specific properties it brings that are crucial for this component. This would improve code maintainability.packages/editor/src/core/extensions/slash-commands/root.tsx (1)
58-58: LGTM: Enhanced type specificity for ReactRendererThe update to use
SlashCommandsMenuPropsin theReactRenderertype improves type checking and consistency. This change is beneficial for maintaining code quality and catching potential issues early.Consider adding a type annotation to the
renderItemsfunction for even better type safety:-const renderItems = () => { +const renderItems = (): { + onStart: (props: { editor: Editor; clientRect?: (() => DOMRect | null) | null }) => void; + onUpdate: (props: { editor: Editor; clientRect?: (() => DOMRect | null) | null }) => void; + onKeyDown: (props: { event: KeyboardEvent }) => boolean; + onExit: () => void; +} => {packages/editor/src/core/extensions/slash-commands/command-menu.tsx (1)
6-9: Approve type definition with a suggestion for improvementThe new
SlashCommandsMenuPropstype enhances the component's type safety and aligns with the PR objectives. However, consider improving the type safety of thecommandproperty.Instead of using
anyfor thecommandproperty, consider defining a more specific type. For example:command: (item: { key: string; [key: string]: any }) => void;This assumes that the
commandfunction takes an item with at least akeyproperty. Adjust the type according to the actual structure of the items if different.packages/editor/src/core/types/editor.ts (1)
18-41: LGTM: Comprehensive TEditorCommands typeThe new
TEditorCommandstype provides a clear and exhaustive list of editor commands, including the new "text-align" command. This enhances type safety and aligns with the PR objectives.Consider using a union of string literals instead of separate lines for better readability:
export type TEditorCommands = | "text" | "h1" | "h2" | "h3" | "h4" | "h5" | "h6" | "bold" | "italic" | "underline" | "strikethrough" | "bulleted-list" | "numbered-list" | "to-do-list" | "quote" | "code" | "table" | "image" | "divider" | "issue-embed" | "text-color" | "background-color" | "text-align";This format is more compact and easier to maintain.
packages/editor/src/core/components/menus/menu-items.ts (2)
208-220: Consider adding explicit typing forcolorparameterWhile the
TextColorItemandBackgroundColorItemnow use the new generic types, thecolorparameter in theisActiveandcommandfunctions could benefit from explicit typing. This would further enhance type safety and prevent potential runtime errors.Consider updating the functions as follows:
isActive: ({ color }: { color: string }) => editor.getAttributes("textStyle").color === color, command: ({ color }: { color: string }) => toggleTextColor(color, editor),Apply similar changes to the
BackgroundColorItemas well.
224-230: New TextAlignItem and updated getEditorMenuItemsThe new
TextAlignItemsuccessfully implements the text alignment feature, and thegetEditorMenuItemsfunction has been updated to maintain type consistency. These changes align well with the PR objectives.Consider adding explicit typing for the
alignmentparameter inTextAlignItemto further enhance type safety:isActive: ({ alignment }: { alignment: 'left' | 'center' | 'right' | 'justify' }) => editor.isActive({ textAlign: alignment }), command: ({ alignment }: { alignment: 'left' | 'center' | 'right' | 'justify' }) => setTextAlign(alignment, editor),This change would help prevent potential errors from invalid alignment values.
Also applies to: 232-232, 256-256
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx (3 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1 hunks)
- packages/editor/src/core/extensions/slash-commands/command-menu.tsx (1 hunks)
- packages/editor/src/core/extensions/slash-commands/root.tsx (2 hunks)
- packages/editor/src/core/extensions/text-align.ts (1 hunks)
- packages/editor/src/core/hooks/use-editor.ts (3 hunks)
- packages/editor/src/core/types/editor.ts (2 hunks)
- space/core/constants/editor.ts (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/editor/src/core/components/menus/bubble-menu/node-selector.tsx
- packages/editor/src/core/extensions/text-align.ts
- packages/editor/src/core/hooks/use-editor.ts
🧰 Additional context used
🔇 Additional comments (23)
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (2)
3-3: LGTM: Import statement updated correctly.The addition of
NodeViewPropsto the import statement is appropriate and necessary for the type definition update that follows. This change enhances type safety in the component.
Line range hint
1-91: Overall assessment: Changes improve type safety without altering functionality.The updates to this file enhance type safety and prepare the component for potential new features related to text alignment. The changes are minimal but impactful, focusing on improving the type definition without altering the existing component logic. This approach maintains current functionality while setting the stage for future enhancements.
To ensure these changes don't introduce any unintended side effects, please run the following verification script:
packages/editor/src/core/extensions/slash-commands/root.tsx (3)
9-9: LGTM: Improved type safety for SlashCommandsMenuThe addition of
SlashCommandsMenuPropsto the import statement enhances type safety for theSlashCommandsMenucomponent. This change aligns well with the PR objectives and is a step towards more robust code.
Line range hint
1-114: Overall assessment: Improved type safety with minimal impactThe changes in this file enhance type safety for the
SlashCommandsMenucomponent without altering the core functionality. These improvements align well with the PR objectives of enhancing the editor's functionality. The suggestions provided are minor and aimed at further improving type safety and code clarity.
9-9: Verify SlashCommandsMenu implementationThe changes to import
SlashCommandsMenuPropsand use it in theReactRenderertype are good improvements. To ensure full compatibility, please verify that theSlashCommandsMenucomponent is correctly implemented with the expected props as defined inSlashCommandsMenuProps.Run the following script to check the
SlashCommandsMenuimplementation:Also applies to: 58-58
packages/editor/src/core/extensions/slash-commands/command-menu.tsx (2)
11-11: Approve component signature updateThe updated
SlashCommandsMenucomponent signature correctly uses the newSlashCommandsMenuPropstype, improving type safety and consistency. This change aligns well with the PR objectives and maintains consistency with the rest of the codebase.
Line range hint
1-124: Verify implementation of text alignment featuresWhile the type and signature changes in this file improve code quality, they don't directly implement the text alignment features mentioned in the PR objectives. Please ensure that the text alignment functionality (Left, Center, Right) and keyboard shortcuts (Cmd + Shift + L/E/R) are implemented in the relevant files.
To verify the implementation of text alignment features, you can run the following script:
This script will help locate the files where text alignment and keyboard shortcuts are implemented.
packages/editor/src/core/types/editor.ts (4)
16-16: LGTM: New import for text alignmentThe addition of the
TTextAlignimport aligns with the PR objective of introducing text alignment features across various editors.
43-56: LGTM: TCommandExtraProps type with text alignmentThe
TCommandExtraPropstype effectively defines additional properties for specific commands, including the new text alignment feature. This enhances type safety and aligns with the PR objectives.Regarding the past review comment about the
alignmentproperty:"text-align": { alignment: TTextAlign; }This implementation already addresses the suggestion to restrict the
alignmentproperty to specific allowed options by using theTTextAligntype. Good job on implementing this improvement!
63-65: LGTM: TCommandWithPropsWithItemKey typeThe
TCommandWithPropsWithItemKeytype is a well-designed extension ofTCommandWithPropsthat includes anitemKeyproperty. This addition enhances type safety and provides a clear structure for commands with associated properties and keys.
89-90: LGTM: Improved type safety in EditorRefApiThe updates to
executeMenuItemCommandandisMenuItemActivemethods in theEditorRefApiinterface significantly improve type safety:executeMenuItemCommand: <T extends TEditorCommands>(props: TCommandWithPropsWithItemKey<T>) => void; isMenuItemActive: <T extends TEditorCommands>(props: TCommandWithPropsWithItemKey<T>) => boolean;These changes ensure that the methods can handle a wide range of command types with their associated properties, aligning well with the new type system introduced earlier in the file.
space/core/constants/editor.ts (9)
2-6: Reconsider the use of the CaseSensitive iconThe
CaseSensitiveicon is imported and later used for the "Text" item in the toolbar. As mentioned in a previous review, this icon might not intuitively represent the "Text" formatting option. Consider using a more appropriate icon likeTextorTypeto improve user understanding.
30-42: Improved type safety and extensibilityThe new
ExtraPropsForCommand<T>utility type and the updatedToolbarMenuItem<T>type with a generic parameter enhance type safety and extensibility. These changes allow for command-specific properties, which is a good improvement.
45-53: Approve renaming and suggest icon changeThe renaming of
BASIC_MARK_ITEMStoTYPOGRAPHY_ITEMSis more descriptive and appropriate. However, as mentioned earlier, reconsider using theCaseSensitiveicon for the "Text" item. A more intuitive icon would improve user understanding.
55-89: Text alignment features implemented correctlyThe
TEXT_ALIGNMENT_ITEMSconstant correctly implements the text alignment features as described in the PR objectives. It includes left, center, and right alignment options with the specified keyboard shortcuts (Cmd + Shift + L/E/R). This implementation enhances the editor's functionality as intended.
91-124: BASIC_MARK_ITEMS updated correctlyThe
BASIC_MARK_ITEMSconstant has been correctly updated to use the newToolbarMenuItemtype with appropriate generic parameters. The functionality of the items remains unchanged, which is good for maintaining consistency.
126-150: LIST_ITEMS updated correctlyThe
LIST_ITEMSconstant has been properly updated to use the newToolbarMenuItemtype with appropriate generic parameters. The functionality of the list items remains consistent, which is good for maintaining the existing behavior.
153-161: USER_ACTION_ITEMS and COMPLEX_ITEMS updated correctlyBoth
USER_ACTION_ITEMSandCOMPLEX_ITEMSconstants have been correctly updated to use the newToolbarMenuItemtype with appropriate generic parameters. The functionality of these items remains unchanged, maintaining consistency in the editor's behavior.
170-177: Text alignment added to all editors correctlyThe
TOOLBAR_ITEMSconstant has been updated to include the new text alignment options for both "lite" and "document" editors. This change aligns with the PR objectives to add text alignment features across all editors. The previous issue with incorrect filtering for the document editor has been resolved.
Line range hint
1-182: Summary of changes and their impactThe changes in this file successfully implement the text alignment features as described in the PR objectives. Key improvements include:
- Addition of text alignment options (left, center, right) with correct keyboard shortcuts.
- Enhanced type safety and extensibility through new type definitions.
- Consistent updates across all relevant constants to support the new features.
These changes will allow users to align text in various editors as intended. The implementation is solid, with only minor suggestions for icon usage improvement.
packages/editor/src/core/components/menus/menu-items.ts (3)
23-23: Improved type safety and new text alignment featureThe changes introduce new imports and type definitions to support text alignment functionality. The use of generic types for
isActiveFunction,commandFunction, andEditorMenuItemsignificantly enhances type safety across the menu items.Also applies to: 30-30, 52-61
63-63: Consistent type improvements across menu itemsAll existing menu item definitions have been updated with specific generic type parameters (e.g.,
EditorMenuItem<"text">,EditorMenuItem<"h1">, etc.). This change enhances type safety and maintains consistency across all menu items.Also applies to: 71-71, 79-79, 87-87, 95-95, 103-103, 111-111, 119-119, 127-127, 135-135, 143-143, 151-151, 159-159, 167-167, 175-175, 183-183, 191-191
199-206: Improved ImageItem implementationThe
ImageItemhas been updated to use the new generic typeEditorMenuItem<"image">. The command function now properly handles cases wheresavedSelectionmight be undefined by providing a fallback position. This addresses the concern raised in a previous review comment about validatingsavedSelection.
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | ||
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | ||
| ? TCommandExtraProps[T] // If the command has extra props, include them | ||
| : object; // Otherwise, just return the command type with no extra props |
There was a problem hiding this comment.
Improve type safety in TCommandWithProps
The TCommandWithProps utility type is a good addition for mapping commands to their extra props. However, there's an opportunity to improve type safety:
export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps
? TCommandExtraProps[T] // If the command has extra props, include them
- : object; // Otherwise, just return the command type with no extra props
+ : Record<string, never>; // Use an explicit empty object typeThis change addresses the past review comment about avoiding {} as a type and uses Record<string, never> for better type safety when representing an empty object.
📝 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.
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | |
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? TCommandExtraProps[T] // If the command has extra props, include them | |
| : object; // Otherwise, just return the command type with no extra props | |
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | |
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? TCommandExtraProps[T] // If the command has extra props, include them | |
| : Record<string, never>; // Use an explicit empty object type |
There was a problem hiding this comment.
@aaryan610, could you please fix this type?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/editor/src/core/components/menus/menu-items.ts (1)
224-230: New TextAlignItem successfully implements text alignment featureThe addition of TextAlignItem successfully implements the text alignment feature mentioned in the PR objectives:
- It correctly uses the
EditorMenuItem<"text-align">type.- The implementation is consistent with other menu items.
- The
isActiveandcommandfunctions use analignmentparameter to determine the current state and set the alignment.To further improve type safety, consider defining a specific type for the alignment options:
type TextAlignment = 'left' | 'center' | 'right' | 'justify'; export const TextAlignItem = (editor: Editor): EditorMenuItem<"text-align"> => ({ // ... isActive: ({ alignment }: { alignment: TextAlignment }) => editor.isActive({ textAlign: alignment }), command: ({ alignment }: { alignment: TextAlignment }) => setTextAlign(alignment, editor), // ... });This change would prevent potential errors from invalid alignment values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx (2 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 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)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/editor/src/core/components/menus/bubble-menu/color-selector.tsx
- packages/editor/src/core/extensions/core-without-props.ts
- packages/editor/src/core/extensions/extensions.tsx
- packages/editor/src/core/extensions/index.ts
- packages/editor/src/core/extensions/read-only-extensions.tsx
🧰 Additional context used
🔇 Additional comments (4)
packages/editor/src/core/components/menus/menu-items.ts (4)
23-23: Excellent improvements in type safety and new text alignment feature!The changes in this section significantly enhance the type safety of the codebase:
- New imports for
AlignCenterandsetTextAlignsupport the text alignment feature.- The introduction of
isActiveFunctionandcommandFunctiontypes provides better type checking for these crucial functions.- The updated
EditorMenuItemtype now uses generics, allowing for more precise type inference based on the specific editor command.These changes will lead to better code completion, fewer runtime errors, and improved maintainability.
Also applies to: 30-30, 52-61
63-222: Consistent type improvements across all menu itemsThe updates to all existing menu item definitions (TextItem, HeadingOneItem, etc.) show a systematic approach to improving type safety:
- Each item now uses the generic
EditorMenuItem<T>type, where T is the specific command type (e.g., "text", "h1", "bold").- This change ensures that the
key,isActive, andcommandproperties for each item are correctly typed according to their specific functionality.- The consistency in these changes across all items reduces the likelihood of type-related errors and improves code maintainability.
Great job on implementing these improvements consistently throughout the file!
199-206: Improved robustness in ImageItem implementationThe changes to the ImageItem definition enhance both type safety and functionality:
- The use of
EditorMenuItem<"image">aligns with the new type system.- The command function now uses object destructuring for
savedSelection, improving code readability.- A fallback is provided for the position when
savedSelectionis undefined, preventing potential runtime errors.command: ({ savedSelection }) => insertImage({ editor, event: "insert", pos: savedSelection?.from ?? editor.state.selection.from }),This fallback mechanism ensures that the image insertion will work correctly even if there's no saved selection, improving the robustness of the feature.
232-232: Successful integration of TextAlignItem in getEditorMenuItemsThe changes to the
getEditorMenuItemsfunction effectively integrate the new text alignment feature:
- The return type
EditorMenuItem<TEditorCommands>[]aligns with the new genericEditorMenuItemtype, ensuring type safety across all menu items.- The addition of
TextAlignItem(editor)to the returned array makes the new text alignment feature available in the editor.These changes complete the implementation of the text alignment feature, making it accessible alongside other editor functionalities.
Also applies to: 256-256
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/editor/src/core/components/menus/bubble-menu/alignment-selector.tsx (1 hunks)
- packages/editor/src/core/components/menus/bubble-menu/root.tsx (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/core/components/menus/bubble-menu/root.tsx
| const textAlignmentOptions: { | ||
| itemKey: TEditorCommands; | ||
| renderKey: string; | ||
| icon: LucideIcon; | ||
| command: () => void; | ||
| isActive: () => boolean; | ||
| }[] = [ | ||
| { | ||
| itemKey: "text-align", | ||
| renderKey: "text-align-left", | ||
| icon: AlignLeft, | ||
| command: () => | ||
| menuItem.command({ | ||
| alignment: "left", | ||
| }), | ||
| isActive: () => | ||
| menuItem.isActive({ | ||
| alignment: "left", | ||
| }), | ||
| }, | ||
| { | ||
| itemKey: "text-align", | ||
| renderKey: "text-align-center", | ||
| icon: AlignCenter, | ||
| command: () => | ||
| menuItem.command({ | ||
| alignment: "center", | ||
| }), | ||
| isActive: () => | ||
| menuItem.isActive({ | ||
| alignment: "center", | ||
| }), | ||
| }, | ||
| { | ||
| itemKey: "text-align", | ||
| renderKey: "text-align-right", | ||
| icon: AlignRight, | ||
| command: () => | ||
| menuItem.command({ | ||
| alignment: "right", | ||
| }), | ||
| isActive: () => | ||
| menuItem.isActive({ | ||
| alignment: "right", | ||
| }), | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring 'textAlignmentOptions' to reduce code duplication
To improve maintainability and readability, you can generate textAlignmentOptions by mapping over an array of alignment configurations. This approach minimizes repetition and makes it easier to add or modify alignment options in the future.
Apply this diff to refactor the code:
+const alignmentOptions = [
+ { alignment: "left", icon: AlignLeft },
+ { alignment: "center", icon: AlignCenter },
+ { alignment: "right", icon: AlignRight },
+];
-const textAlignmentOptions: {
- itemKey: TEditorCommands;
- renderKey: string;
- icon: LucideIcon;
- command: () => void;
- isActive: () => boolean;
-}[] = [
- {
- itemKey: "text-align",
- renderKey: "text-align-left",
- icon: AlignLeft,
- command: () =>
- menuItem.command({
- alignment: "left",
- }),
- isActive: () =>
- menuItem.isActive({
- alignment: "left",
- }),
- },
- // ... other alignment options
-];
+const textAlignmentOptions = alignmentOptions.map((option) => ({
+ itemKey: "text-align" as TEditorCommands,
+ renderKey: `text-align-${option.alignment}`,
+ icon: option.icon,
+ command: () =>
+ menuItem.command({
+ alignment: option.alignment,
+ }),
+ isActive: () =>
+ menuItem.isActive({
+ alignment: option.alignment,
+ }),
+}));📝 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.
| const textAlignmentOptions: { | |
| itemKey: TEditorCommands; | |
| renderKey: string; | |
| icon: LucideIcon; | |
| command: () => void; | |
| isActive: () => boolean; | |
| }[] = [ | |
| { | |
| itemKey: "text-align", | |
| renderKey: "text-align-left", | |
| icon: AlignLeft, | |
| command: () => | |
| menuItem.command({ | |
| alignment: "left", | |
| }), | |
| isActive: () => | |
| menuItem.isActive({ | |
| alignment: "left", | |
| }), | |
| }, | |
| { | |
| itemKey: "text-align", | |
| renderKey: "text-align-center", | |
| icon: AlignCenter, | |
| command: () => | |
| menuItem.command({ | |
| alignment: "center", | |
| }), | |
| isActive: () => | |
| menuItem.isActive({ | |
| alignment: "center", | |
| }), | |
| }, | |
| { | |
| itemKey: "text-align", | |
| renderKey: "text-align-right", | |
| icon: AlignRight, | |
| command: () => | |
| menuItem.command({ | |
| alignment: "right", | |
| }), | |
| isActive: () => | |
| menuItem.isActive({ | |
| alignment: "right", | |
| }), | |
| }, | |
| ]; | |
| const alignmentOptions = [ | |
| { alignment: "left", icon: AlignLeft }, | |
| { alignment: "center", icon: AlignCenter }, | |
| { alignment: "right", icon: AlignRight }, | |
| ]; | |
| const textAlignmentOptions = alignmentOptions.map((option) => ({ | |
| itemKey: "text-align" as TEditorCommands, | |
| renderKey: `text-align-${option.alignment}`, | |
| icon: option.icon, | |
| command: () => | |
| menuItem.command({ | |
| alignment: option.alignment, | |
| }), | |
| isActive: () => | |
| menuItem.isActive({ | |
| alignment: option.alignment, | |
| }), | |
| })); |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/editor/src/core/extensions/index.ts (1)
19-19: Consider maintaining alphabetical ordering of exports.The new exports break the alphabetical ordering pattern. Consider reordering them for better maintainability.
Apply this reordering:
export * from "./enter-key-extension"; export * from "./extensions"; -export * from "./headers"; export * from "./horizontal-rule"; export * from "./keymap"; export * from "./quote"; export * from "./read-only-extensions"; export * from "./side-menu"; -export * from "./text-align"; +export * from "./headers"; +export * from "./text-align";Also applies to: 25-25
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1)
Line range hint
58-58: Consider fixing the type definition instead of suppressing the error.The
@ts-expect-errorcomment suggests that thegetImageSourcecommand's type definition might be missing or incorrect in the editor's command interface.Consider properly typing the editor commands by extending the editor's command interface:
declare module '@tiptap/core' { interface Commands<ReturnType> { imageComponent: { getImageSource: (src: string) => ReturnType; } } }space/core/constants/editor.ts (2)
55-89: Consider improving keyboard shortcuts for better accessibility.Two suggestions for the text alignment shortcuts:
- The center alignment shortcut
Cmd+Shift+Eis not intuitive. Consider usingCmd+Shift+Cinstead.- The shortcuts use
Cmdwhich is Mac-specific. Consider using a cross-platform approach.Apply this diff to improve the shortcuts:
{ itemKey: "text-align", renderKey: "text-align-center", name: "Center align", icon: AlignCenter, - shortcut: ["Cmd", "Shift", "E"], + shortcut: ["⌘/Ctrl", "Shift", "C"], editors: ["lite", "document"], extraProps: { alignment: "center", }, },
55-89: Consider adding JSDoc documentation for the text alignment configuration.While the implementation is solid, adding JSDoc comments would help document:
- The purpose and behavior of each alignment option
- The expected behavior when alignment is applied to different content types
- Any limitations or edge cases
Example documentation:
/** * Configuration for text alignment toolbar items. * Supports left, center, and right alignment for both lite and document editors. * * @remarks * - Alignment is applied to the current block or selection * - For tables, alignment is applied to the current cell * - For lists, alignment is applied to the entire list item */ export const TEXT_ALIGNMENT_ITEMS: ToolbarMenuItem<"text-align">[] = [web/core/constants/editor.ts (3)
35-39: Consider using a more restrictive type for the default case.The
ExtraPropsForCommandutility type usesobjectas a fallback, which is very permissive. Consider usingRecord<string, never>or{}to be more explicit about empty objects.type ExtraPropsForCommand<T extends TEditorCommands> = T extends keyof TCommandExtraProps ? TCommandExtraProps[T] - : object; // Default to empty object for commands without extra props + : Record<string, never>; // More restrictive empty object type
60-94: Consider adding ARIA labels for accessibility.While the implementation is solid, consider enhancing accessibility by adding ARIA labels to help screen readers announce the current text alignment state.
{ itemKey: "text-align", renderKey: "text-align-left", name: "Left align", icon: AlignLeft, shortcut: ["Cmd", "Shift", "L"], editors: ["lite", "document"], extraProps: { alignment: "left", + ariaLabel: "Align text to the left", + ariaPressed: "false" // To be controlled by the component }, },
Line range hint
173-186: Consider extracting the filter logic to reduce duplication.The filter logic is repeated for each editor type. Consider extracting it into a utility function.
+const filterToolbarItemsByEditor = (items: ToolbarMenuItem[], editorType: TEditorTypes) => + items.filter((item) => item.editors.includes(editorType)); export const TOOLBAR_ITEMS: { [editorType in TEditorTypes]: Record<ToolbarSection, ToolbarMenuItem<TEditorCommands>[]>; } = { lite: { - basic: BASIC_MARK_ITEMS.filter((item) => item.editors.includes("lite")), - alignment: TEXT_ALIGNMENT_ITEMS.filter((item) => item.editors.includes("lite")), + basic: filterToolbarItemsByEditor(BASIC_MARK_ITEMS, "lite"), + alignment: filterToolbarItemsByEditor(TEXT_ALIGNMENT_ITEMS, "lite"), // ... apply to other sections }, document: { // Similar changes for document editor } };
📜 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 (12)
- packages/editor/package.json (1 hunks)
- packages/editor/src/core/components/menus/menu-items.ts (3 hunks)
- packages/editor/src/core/extensions/core-without-props.ts (2 hunks)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx (1 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/helpers/editor-commands.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (2 hunks)
- packages/editor/src/styles/editor.css (1 hunks)
- space/core/constants/editor.ts (3 hunks)
- web/core/constants/editor.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/editor/package.json
- packages/editor/src/core/extensions/core-without-props.ts
- packages/editor/src/core/extensions/extensions.tsx
- packages/editor/src/core/extensions/read-only-extensions.tsx
- packages/editor/src/core/helpers/editor-commands.ts
- packages/editor/src/styles/editor.css
🔇 Additional comments (15)
packages/editor/src/core/extensions/index.ts (1)
25-25: Verify text alignment implementation.The new text-align export aligns with the PR objectives for implementing text alignment features. Let's verify the implementation details:
✅ Verification successful
Text alignment implementation is properly configured and functional
The text alignment extension is well-implemented with:
- Complete configuration using
@tiptap/extension-text-alignwith left, center, and right alignment options- Proper keyboard shortcuts (Cmd/Ctrl + Shift + L/E/R) for left, center, and right alignment respectively
- Support for both heading and paragraph nodes
- Integration with editor toolbar and menu items
- Consistent implementation across editor types (lite and document)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the text-align extension implementation # Look for keyboard shortcut handlers and alignment options # Check for text alignment related code rg -A 5 "text-align" --type ts --type tsx # Check for keyboard shortcut implementation rg -A 5 "(Cmd|Ctrl)\s*\+\s*Shift\s*\+\s*(L|E|R)" --type ts --type tsxLength of output: 169
Script:
#!/bin/bash # Let's try a different approach to check the text-align implementation # Check the text-align extension file content fd "text-align.ts" --exec cat {} # Search for alignment-related code without file type restrictions rg -A 5 "text-align" # Search for keyboard shortcuts without file type restrictions rg -A 5 "(Cmd|Ctrl).*Shift.*(L|E|R)"Length of output: 106919
packages/editor/src/core/extensions/custom-image/components/image-node.tsx (2)
Line range hint
52-52: Verify image alignment behavior with new text alignment features.Since this PR implements text alignment features, ensure that images respect their parent block's alignment settings. The
NodeViewWrappermight need additional styling or attributes to handle alignment properly.Let's check if there are any alignment-related styles or attributes being applied to images:
#!/bin/bash # Search for alignment-related code in image components rg -A 5 "align|text-align" --type typescript --type css packages/editor/src/core/extensions/custom-image/
Line range hint
6-13: Type consolidation improves clarity and type safety.The consolidation of type definitions into
CustomImageNodeViewPropsenhances type safety by:
- Directly extending
NodeViewProps- Explicitly typing the node attributes through the
ImageAttributesinterface- Properly typing the editor instance and position getter
Let's verify if this type is consistently used across the codebase:
✅ Verification successful
Type consolidation is correctly implemented and used consistently
The verification shows that
CustomImageNodeViewPropsis:
- Defined in
image-node.tsxand properly used in itsCustomImageNodecomponent- Extended in
image-block.tsxforCustomImageBlockProps- Correctly integrated with
ImageAttributestype which is used across multiple files in the custom-image extensionThe type consolidation is working as intended and maintains type safety across the image-related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other components using CustomImageNodeViewProps rg "CustomImageNodeViewProps" --type typescriptLength of output: 142
Script:
#!/bin/bash # Search for CustomImageNodeViewProps usage with ripgrep rg -A 5 "CustomImageNodeViewProps" # Also search for files containing ImageAttributes to understand type usage rg -A 3 "ImageAttributes" # Use ast-grep to find React components that might be using these props ast-grep --pattern 'const $_ = ($props: CustomImageNodeViewProps) => { $$$ }'Length of output: 6978
packages/editor/src/core/types/editor.ts (4)
18-42: LGTM! Well-structured command type definition.The
TEditorCommandstype provides a comprehensive list of editor commands, including the new text alignment feature, while maintaining a clean and logical organization.
44-57: LGTM! Well-defined command properties.The
TCommandExtraPropstype properly defines the extra properties needed for each command, including the new text alignment feature using the importedTTextAligntype.
90-91: LGTM! Type-safe method signatures.The updated method signatures in
EditorRefApiproperly utilize the new generic types, ensuring type safety when executing commands and checking their active state.
Line range hint
18-91: Verify keyboard shortcut implementation.The type definitions support the text alignment feature. Let's verify the keyboard shortcut implementation for text alignment commands.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Text alignment keyboard shortcuts are properly implemented.
The implementation includes:
- Text alignment keyboard shortcuts defined in
space/core/constants/editor.ts:
Cmd + Shift + Lfor left alignmentCmd + Shift + Efor center alignmentCmd + Shift + Rfor right alignment- Text alignment extension configured in
packages/editor/src/core/extensions/text-align.ts- Menu items and commands properly set up in
packages/editor/src/core/components/menus/menu-items.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for keyboard shortcut handlers for text alignment # Search for keyboard shortcut definitions echo "Searching for text alignment keyboard shortcuts..." rg -l "Cmd \+ Shift \+ [LER]|text-align" --type ts # Search for specific shortcut handlers echo "Searching for specific shortcut implementations..." ast-grep --pattern 'handleKeyDown($$$) { $$$ text-align $$$ }'Length of output: 732
Script:
#!/bin/bash # Let's check the text alignment extension implementation echo "Checking text-align extension implementation..." cat packages/editor/src/core/extensions/text-align.ts # Check keyboard shortcuts configuration echo -e "\nChecking keyboard shortcuts configuration..." cat packages/editor/src/core/extensions/core-without-props.ts # Check menu items configuration echo -e "\nChecking menu items configuration..." cat packages/editor/src/core/components/menus/menu-items.tsLength of output: 10786
Script:
#!/bin/bash # Let's check for keyboard shortcut configurations in editor constants echo "Checking keyboard shortcuts in editor constants..." cat space/core/constants/editor.ts # Also check for any keyboard event handlers echo -e "\nChecking for keyboard event handlers..." rg -A 5 "handleKeyDown|onKeyDown" --type tsLength of output: 57799
space/core/constants/editor.ts (3)
30-33: Well-designed utility type for command properties!The
ExtraPropsForCommandutility type elegantly handles optional command properties using conditional types. This ensures type safety while maintaining flexibility.
35-42: Excellent type safety improvements!The generic
ToolbarMenuItemtype with separateitemKeyandrenderKeyproperties improves type safety and clarity. The generic constraint ensures that only valid editor commands can be used.
170-170: LGTM! Correct editor type filtering.The text alignment items are now properly filtered for both
liteanddocumenteditor types, addressing the issues raised in previous reviews.Also applies to: 177-177
web/core/constants/editor.ts (1)
96-165: Clean and consistent refactoring of toolbar items.The updates to existing toolbar items maintain consistency and type safety while preserving functionality.
packages/editor/src/core/components/menus/menu-items.ts (4)
203-204: Validate 'savedSelection' inImageItem'scommandFunctionThe previous comment regarding type safety and validation of the
savedSelectionparameter in theImageItem'scommandfunction is still applicable.
211-212: Ensure 'color' Parameter Is Properly Typed inTextColorItemThe previous comment about adding explicit type definitions for the
colorparameter inTextColorItemis still valid.
219-220: Ensure 'color' Parameter Is Properly Typed inBackgroundColorItemThe previous comment about adding explicit type definitions for the
colorparameter inBackgroundColorItemis still valid.
224-230: Enhance Type Safety for 'alignment' Parameter inTextAlignItemThe previous comment regarding the need to define a specific type for the
alignmentparameter inTextAlignItemis still applicable. Defining a type foralignmentwill improve type safety and prevent potential misuse.
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | ||
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | ||
| ? TCommandExtraProps[T] // If the command has extra props, include them | ||
| : object; // Otherwise, just return the command type with no extra props | ||
|
|
||
| type TCommandWithPropsWithItemKey<T extends TEditorCommands> = T extends keyof TCommandExtraProps | ||
| ? { itemKey: T } & TCommandExtraProps[T] | ||
| : { itemKey: T }; |
There was a problem hiding this comment.
Improve type safety by using a more specific type.
The use of object type is too broad and could lead to unexpected behavior. Consider using a more specific type.
Apply this diff to improve type safety:
export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps
? TCommandExtraProps[T] // If the command has extra props, include them
- : object; // Otherwise, just return the command type with no extra props
+ : Record<string, never>; // Use an explicit empty object type📝 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.
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | |
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? TCommandExtraProps[T] // If the command has extra props, include them | |
| : object; // Otherwise, just return the command type with no extra props | |
| type TCommandWithPropsWithItemKey<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? { itemKey: T } & TCommandExtraProps[T] | |
| : { itemKey: T }; | |
| // Create a utility type that maps a command to its extra props or an empty object if none are defined | |
| export type TCommandWithProps<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? TCommandExtraProps[T] // If the command has extra props, include them | |
| : Record<string, never>; // Use an explicit empty object type | |
| type TCommandWithPropsWithItemKey<T extends TEditorCommands> = T extends keyof TCommandExtraProps | |
| ? { itemKey: T } & TCommandExtraProps[T] | |
| : { itemKey: T }; |
|
|
||
| export const TOOLBAR_ITEMS: { | ||
| [editorType in TEditorTypes]: { | ||
| [key: string]: ToolbarMenuItem[]; | ||
| [key: string]: ToolbarMenuItem<TEditorCommands>[]; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a typed record for toolbar sections.
The toolbar section keys are currently stringly typed. Consider creating a union type for better type safety.
+type ToolbarSection = 'basic' | 'alignment' | 'list' | 'userAction' | 'complex';
export const TOOLBAR_ITEMS: {
[editorType in TEditorTypes]: {
- [key: string]: ToolbarMenuItem<TEditorCommands>[];
+ [key in ToolbarSection]: ToolbarMenuItem<TEditorCommands>[];
};
}📝 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.
| export const TOOLBAR_ITEMS: { | |
| [editorType in TEditorTypes]: { | |
| [key: string]: ToolbarMenuItem[]; | |
| [key: string]: ToolbarMenuItem<TEditorCommands>[]; | |
| }; | |
| type ToolbarSection = 'basic' | 'alignment' | 'list' | 'userAction' | 'complex'; | |
| export const TOOLBAR_ITEMS: { | |
| [editorType in TEditorTypes]: { | |
| [key in ToolbarSection]: ToolbarMenuItem<TEditorCommands>[]; | |
| }; | |
| } |
| type isActiveFunction<T extends TEditorCommands> = (params?: TCommandWithProps<T>) => boolean; | ||
| type commandFunction<T extends TEditorCommands> = (params?: TCommandWithProps<T>) => void; | ||
|
|
||
| export type EditorMenuItem<T extends TEditorCommands> = { | ||
| key: T; | ||
| name: string; | ||
| command: (...args: any) => void; | ||
| command: commandFunction<T>; | ||
| icon: LucideIcon; | ||
| } & ( | ||
| | { | ||
| key: TNonColorEditorCommands; | ||
| isActive: () => boolean; | ||
| } | ||
| | { | ||
| key: TColorEditorCommands; | ||
| isActive: (color: string | undefined) => boolean; | ||
| } | ||
| ); | ||
|
|
||
| export const TextItem = (editor: Editor): EditorMenuItem => ({ | ||
| isActive: isActiveFunction<T>; | ||
| }; |
There was a problem hiding this comment.
Ensure Consistency in Type Definitions for 'isActive' and 'command' Functions
The new type definitions for isActiveFunction and commandFunction expect functions that accept params?: TCommandWithProps<T>. However, many of the isActive and command functions in the EditorMenuItem instances do not accept any parameters. This could lead to type mismatches or TypeScript errors.
Consider updating the isActive and command functions to accept params?: TCommandWithProps<T> to ensure type safety and consistency with the defined types.
Example fix:
export const BoldItem = (editor: Editor): EditorMenuItem<"bold"> => ({
key: "bold",
name: "Bold",
- isActive: () => editor.isActive("bold"),
+ isActive: (params) => editor.isActive("bold"),
- command: () => toggleBold(editor),
+ command: (params) => toggleBold(editor),
icon: BoldIcon,
});Apply similar changes to other menu items where the functions do not accept parameters.
Committable suggestion was skipped due to low confidence.
Users can now use
Left,CenterandRighttext alignments on pages, issue descriptions and comments.Ways to align text-
Cmd + Shift + L/E/Rfor left, center and right respectively.Media:
Screen.Recording.2024-10-16.at.18.51.18.mov
Summary by CodeRabbit
New Features
TextAlignmentSelectorcomponent for selecting text alignment (left, center, right) in the text editor.CustomTextAlignExtension.Enhancements
Bug Fixes
AddCommentcomponent's placeholder text for clarity.Documentation