[PE-68] fix: markdown transformation of mention and custom image components#5864
[PE-68] fix: markdown transformation of mention and custom image components#5864SatishGandham merged 3 commits intopreviewfrom
Conversation
WalkthroughThe pull request introduces enhancements to several extensions within the editor package, focusing on Markdown serialization for images and mentions. The 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: 7
🧹 Outside diff range and nitpick comments (4)
web/ce/components/pages/editor/embed/issue-embed-upgrade-card.tsx (2)
10-15: LGTM: Improved styling approach with a minor suggestionThe use of the
cnfunction for conditional styling is a good practice. It improves readability and maintainability of the component's styles.Consider using a Tailwind CSS class for the border width instead of a custom value:
- "border-[0.5px] border-custom-border-200", + "border border-custom-border-200",This change would align better with Tailwind's utility-first approach and maintain consistency with the rest of the codebase.
24-31: LGTM: Improved CTA implementation with a suggestionThe replacement of the
Buttoncomponent with an anchor tag is appropriate for external links. The use ofgetButtonStylingensures consistent button styling across the application.Consider adding an
aria-labelto the anchor tag to improve accessibility:<a href="https://plane.so/pro" target="_blank" rel="noopener noreferrer" + aria-label="Upgrade to Plane Pro" className={cn(getButtonStyling("primary", "md"), "no-underline")} > Upgrade </a>This addition will provide more context to screen reader users about the purpose of the link.
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (1)
61-61: Consider including theheightattribute in serialized outputCurrently, only the
srcandwidthattributes are included in the serialized<img>tag. If theheightattribute is relevant for image rendering, consider including it in the output to ensure consistent display across different platforms.Apply the following diff to include the
heightattribute:- state.write(`<img src="${imageSource}" width="${attrs.width}" />`); + state.write(`<img src="${imageSource}" width="${attrs.width}" height="${attrs.height}" />`);packages/editor/src/core/extensions/custom-image/custom-image.ts (1)
128-130: Consider includingaspectRatioin the serialized outputSince you've added an
aspectRatioattribute to the image node, you might want to include it in the serialized Markdown output if it's necessary for reconstructing the image during deserialization.Suggestion:
Include the
aspectRatioattribute in the<img>tag.- state.write(`<img src="${imageSource}" width="${attrs.width}" />`); + state.write(`<img src="${imageSource}" width="${attrs.width}" aspectRatio="${attrs.aspectRatio}" />`);This will ensure that the
aspectRatiois preserved during the serialization and deserialization process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (2 hunks)
- packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (2 hunks)
- packages/editor/src/core/extensions/mentions/extension.tsx (2 hunks)
- web/ce/components/pages/editor/embed/issue-embed-upgrade-card.tsx (1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint-web
web/ce/components/pages/editor/embed/issue-embed-upgrade-card.tsx
[warning] 8-8:
Unexpected any. Specify a different type
🔇 Additional comments (8)
web/ce/components/pages/editor/embed/issue-embed-upgrade-card.tsx (2)
1-2: LGTM: Improved import for button stylingThe addition of
getButtonStylingfrom "@plane/ui" is a good practice. It centralizes button styling, which can lead to more consistent UI across the application and easier maintenance.Also applies to: 5-6
17-22: LGTM: Improved content structureThe simplification of the inner content structure enhances readability and reduces unnecessary DOM nesting. The use of a
ptag for the description text is semantically correct and improves accessibility.packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (3)
3-4: Imports added for Markdown serializationThe imports for
MarkdownSerializerStateandNodeare correctly added to support the Markdown serialization functionality.
7-7: ImportingImageAttributesThe import of
ImageAttributesfrom@/extensions/custom-imageis appropriate and used correctly in the code.
57-64: Enhancing image handling with Markdown serializationThe addition of the
markdownstorage with aserializemethod appropriately enhances image handling by enabling Markdown serialization for custom images.packages/editor/src/core/extensions/mentions/extension.tsx (1)
3-4: Imports for Markdown serialization are appropriateThe addition of
MarkdownSerializerStateandNodeimports is necessary for implementing the custom Markdown serialization functionality.packages/editor/src/core/extensions/custom-image/custom-image.ts (2)
3-4: Imports are appropriateThe imports of
MarkdownSerializerStateandNodeare necessary for implementing the Markdown serialization functionality.
8-8: Import statement for custom typesImporting
CustomImageNodeandImageAttributesfrom@/extensions/custom-imageensures that the custom image node and its attributes are correctly referenced.
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/editor/src/core/extensions/custom-image/custom-image.ts (2 hunks)
- packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (2 hunks)
- packages/editor/src/core/extensions/mentions/extension.tsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/editor/src/core/extensions/custom-image/custom-image.ts
- packages/editor/src/core/extensions/mentions/extension.tsx
🧰 Additional context used
🔇 Additional comments (3)
packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts (3)
3-4: LGTM: New imports for markdown serializationThe new imports for
MarkdownSerializerStateandNodeare correctly added and necessary for the new markdown serialization feature.
7-7: LGTM: Updated import for ImageAttributesThe import statement has been correctly updated to include
ImageAttributes, which is likely used in the new markdown serialization feature.
57-65: LGTM: New markdown serialization for imagesThe new
markdownobject with itsserializemethod is well-implemented. It correctly constructs an<img>tag with the necessary attributes, uses proper escaping, and maintains consistent image source resolution with the existing code.
| markdown: { | ||
| serialize(state: MarkdownSerializerState, node: Node) { | ||
| const attrs = node.attrs as ImageAttributes; | ||
| const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src) || attrs.src); |
There was a problem hiding this comment.
Fix invocation of getImageSource command
The current implementation doesn't address the issue raised in a previous review. The getImageSource command returns a function that needs to be invoked to get the actual image source. Currently, the function is not being called, which may result in imageSource being assigned a function instead of the expected string.
Apply the following diff to fix the issue:
- const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src) || attrs.src);
+ const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src)?.() || attrs.src);📝 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 imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src) || attrs.src); | |
| const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src)?.() || attrs.src); |
There was a problem hiding this comment.
Can you check this issue. getImageSource is returning a function here
There was a problem hiding this comment.
@SatishGandham, thank you for bringing this to my attention. You're correct; in read-only-custom-image.ts, the getImageSource command returns a string directly, not a function. Therefore, the current implementation is correct, and my previous suggestion isn't necessary. I apologize for the oversight.
(>_<) Sorry about that.
✏️ Learnings added
Learnt from: SatishGandham
PR: makeplane/plane#5864
File: packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts:60-60
Timestamp: 2024-10-22T08:03:04.172Z
Learning: In `packages/editor/src/core/extensions/custom-image/read-only-custom-image.ts`, the `getImageSource` command returns a string directly, not a function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| markdown: { | ||
| serialize(state: MarkdownSerializerState, node: Node) { | ||
| const attrs = node.attrs as ImageAttributes; | ||
| const imageSource = state.esc(this?.editor?.commands?.getImageSource?.(attrs.src) || attrs.src); |
There was a problem hiding this comment.
Can you check this issue. getImageSource is returning a function here
3 participants
Improvements:
Summary by CodeRabbit
New Features
IssueEmbedUpgradeCardfor improved layout and dynamic styling.Bug Fixes
Refactor
IssueEmbedUpgradeCardcomponent for better readability and maintainability.IssueEmbedUpgradeCardcomponent.