Fix rte source mode #5083
Fix rte source mode #5083
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @whoselen - I've reviewed your changes and they look great!
General suggestions:
- Ensure consistency in handling the 'style' attribute across all extensions to maintain uniformity.
- Consider refactoring repeated logic for parsing and rendering the 'style' attribute into a shared utility function to improve code maintainability.
- Review the implementation of dynamic height adjustments in the ProseMirrorWrapper for potential simplification.
- Verify that the updates to tiptap dependencies do not introduce any regressions or compatibility issues with existing editor features.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 6 issues found
- 🟡 Complexity: 1 issue found
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| addAttributes() { | ||
| return { | ||
| href: { | ||
| default: null, | ||
| }, | ||
| target: { | ||
| default: this.options.HTMLAttributes.target, | ||
| }, | ||
| rel: { | ||
| default: this.options.HTMLAttributes.rel, | ||
| }, | ||
| style: { | ||
| parseHTML: (element) => element.getAttribute('style'), | ||
| renderHTML: (attributes) => { | ||
| if (!attributes.style) { | ||
| return {}; | ||
| } | ||
| return { style: attributes.style }; | ||
| }, | ||
| }, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
suggestion (code_refinement): Consider refactoring to a shared utility function for style attribute parsing and rendering.
The logic for parsing and rendering the 'style' attribute is repeated across multiple extensions. A shared utility function could reduce redundancy and improve maintainability.
| } | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
suggestion (code_refinement): Ensure consistency in handling 'style' attribute across all extensions.
The addition of 'style' attribute handling in the Image extension is a good improvement. Ensure similar attributes are consistently handled across all relevant extensions for uniformity.
| @@ -0,0 +1,37 @@ | |||
| import { Extension } from '@tiptap/core'; | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for GlobalAttributes extension.
It's important to ensure that the GlobalAttributes extension correctly parses and renders HTML attributes as expected. This could include testing various HTML elements with different attributes to verify that they are handled correctly.
| @@ -0,0 +1,30 @@ | |||
| import { Mark, mergeAttributes } from '@tiptap/core'; | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for SpanMark mark.
Testing the SpanMark mark is crucial to ensure that it correctly parses and renders span elements with styles. This could involve verifying that the style attributes are correctly applied and rendered in the output HTML.
| @@ -0,0 +1,29 @@ | |||
| import { Node } from '@tiptap/core'; | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for DivNode node.
Ensuring that the DivNode node behaves as expected is important, especially in terms of parsing and rendering div elements with styles and alignment attributes. Tests could validate the correct handling of these attributes.
| @@ -22,9 +22,17 @@ const RichTextEditorWrapper = styled.div<{ $position: string }>` | |||
| } | |||
| `; | |||
|
|
|||
| const ProseMirrorWrapper = styled.div` | |||
| const ProseMirrorWrapper = styled.div<{ | |||
There was a problem hiding this comment.
issue (complexity): Consider simplifying the implementation by using default props in the styled component's template literals.
The introduction of conditional logic for setting height, min-height, and max-height based on props significantly increases the complexity of this component. While the added flexibility is appreciated, it also makes the component more dependent on external props and introduces additional cognitive load to understand its styling behavior.
To simplify, consider using default props directly in the styled component's template literals. This approach can reduce complexity while maintaining the intended functionality. For example:
const ProseMirrorWrapper = styled.div`
overflow-y: auto;
height: ${({ $height = 'unset' }) => $height};
min-height: ${({ $minHeight = 'unset' }) => $minHeight};
max-height: ${({ $maxHeight = 'unset' }) => $maxHeight};
resize: vertical;
`;This method streamlines the implementation by removing the need for conditional logic and makes the component's behavior more straightforward to understand at a glance.
| const width = | ||
| element.style.width || element.getAttribute('width') || null; | ||
| return width; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
| const width = | |
| element.style.width || element.getAttribute('width') || null; | |
| return width; | |
| return element.style.width || element.getAttribute('width') || null; | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| { | ||
| overflow-y: auto; | ||
| height: ${(props) => (props.$height ? props.$height : 'unset')}; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
| height: ${(props) => (props.$height ? props.$height : 'unset')}; | |
| height: ${(props) => (props.$height || 'unset')}; |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
| { | ||
| overflow-y: auto; | ||
| height: ${(props) => (props.$height ? props.$height : 'unset')}; | ||
| min-height: ${(props) => (props.$minHeight ? props.$minHeight : 'unset')}; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
| min-height: ${(props) => (props.$minHeight ? props.$minHeight : 'unset')}; | |
| min-height: ${(props) => (props.$minHeight || 'unset')}; |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
| { | ||
| overflow-y: auto; | ||
| height: ${(props) => (props.$height ? props.$height : 'unset')}; | ||
| min-height: ${(props) => (props.$minHeight ? props.$minHeight : 'unset')}; | ||
| max-height: ${(props) => (props.$maxHeight ? props.$maxHeight : 'unset')}; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
| max-height: ${(props) => (props.$maxHeight ? props.$maxHeight : 'unset')}; | |
| max-height: ${(props) => (props.$maxHeight || 'unset')}; |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
|
ISSUE
Context
Make editor have tiptap schema for customizable source mode input
// Delete the below section once completed
PR Checklist