feat(text-editor): add support for pasting inline images #3464
Conversation
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3464/ |
src/components/text-editor/prosemirror-adapter/plugins/image/wrapper-node.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/resize-plugin.ts
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
3 times, most recently
from
9bc1794 to
c623437
Compare
FredrikWallstrom
changed the title
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
68fc5da to
90e8bc3
Compare
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe changes implement enhanced image handling capabilities across the project. New TypeScript interfaces, enums, and event handlers have been introduced to manage image states and actions such as pasting and removing images. Several ProseMirror plugins and utilities were added for image insertion, node specification, and view rendering, while a legacy image remover plugin was removed. The text editor and its adapter now emit image-related events, the HTML and Markdown converters account for image nodes, and translation files include new entries for image upload accessibility. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TextEditor
participant ProsemirrorAdapter
participant ImageInserterPlugin
participant ImageViewPlugin
User->>TextEditor: Paste image
TextEditor->>ProsemirrorAdapter: Trigger paste event
ProsemirrorAdapter->>ImageInserterPlugin: Process image data
ImageInserterPlugin-->>ProsemirrorAdapter: Emit imagePasted event
ProsemirrorAdapter->>TextEditor: Propagate imagePasted event
TextEditor->>ImageViewPlugin: Render image node (loading → success/failure)
sequenceDiagram
participant User
participant TextEditor
participant ProsemirrorAdapter
participant ImageViewPlugin
User->>TextEditor: Remove image
TextEditor->>ProsemirrorAdapter: Trigger removal event
ProsemirrorAdapter->>ImageViewPlugin: Process node removal
ProsemirrorAdapter->>TextEditor: Emit imageRemoved event with image details
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 23
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
etc/lime-elements.api.md(3 hunks)src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx(1 hunks)src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.scss(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(5 hunks)src/components/text-editor/text-editor.tsx(5 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/html-converter.ts(2 hunks)src/components/text-editor/utils/markdown-converter.ts(3 hunks)src/global/translations.ts(1 hunks)src/translations/en.ts(1 hunks)src/translations/sv.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx
[error] 27-27: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 31-31: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 34-34: This type annotation is trivially inferred from its initialization.
Safe fix: Remove the type annotation.
(lint/style/noInferrableTypes)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
[error] 314-314: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (41)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss (1)
172-172: Added support for image styling - great!The import for the image view styles is correctly placed after other imports, which helps maintain a clean separation of concerns. This file will contain the necessary styles for the new image functionality.
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (3)
1-25: Component structure looks goodThe component is well-structured with proper imports and documentation. The JSDoc comments clearly explain the purpose and usage of this example.
29-38: LGTM!The render method is clean and properly configures the text editor with the necessary props and event handlers.
40-42: LGTM!The change handler is correctly implemented using the event detail.
src/global/translations.ts (1)
21-36: Well implemented parameterized translationsThe implementation of parameterized translations is clean and effective:
- The regex pattern correctly identifies placeholders like
{paramName}in translation strings- The method signature is updated properly with an optional params parameter
- The code handles missing translations and unmatched placeholders gracefully
This is a valuable addition that will support the new image-related UI elements with proper localization.
src/components/text-editor/text-editor.types.ts (4)
1-2: LGTM!Good addition of the FileInfo import which is necessary for the new image handling interfaces.
66-92: The ImageInserter interface is well-designedThe interface provides a clean API for inserting images and handling different states. The methods are well-documented and have clear purposes:
insertThumbnail()for initial placeholderinsertImage(src?)for the final image (with optional URL)insertFailedThumbnail()for error state
94-101: LGTM!The ImageState enum has appropriate values for tracking the lifecycle of an image (loading, failed, success).
103-120: The ImageInfo interface is well-structuredThe interface provides all necessary properties to track and display an image:
- fileInfoId for unique identification
- src for the image source (URL or base64)
- state for tracking the current status
The documentation is clear and comprehensive.
src/components/text-editor/text-editor.tsx (7)
6-11: Well-structured import groupingThe imports are nicely organized, adding the necessary types for the new image-handling functionality.
28-29: Good documentation with example referencesThe JSDoc has been properly updated to include references to the new examples showcasing the inline image functionality, which will help developers understand how to use this feature.
171-178: Event naming and typing looks goodThe
imagePastedevent is properly defined with appropriate JSDoc comments and typing using theImageInserterinterface. The alpha and private tags are correctly applied since this is a new feature.
180-187: Event naming and typing looks goodThe
imageRemovedevent is properly defined with appropriate JSDoc comments and typing using theImageInfointerface. The alpha and private tags are correctly applied since this is a new feature.
268-269: Handlers properly connected to the adapter componentThe new image event handlers are correctly wired up to the ProseMirror adapter component, maintaining consistency with the existing pattern for event handling.
338-341: Handler implementation follows established patternThe
handleImagePastedmethod follows the same pattern as other event handlers in this component: stopping propagation and emitting the event with the original event detail.
343-346: Handler implementation follows established patternThe
handleImageRemovedmethod follows the same pattern as other event handlers in this component: stopping propagation and emitting the event with the original event detail.src/components/text-editor/utils/html-converter.ts (2)
5-5: Appropriate import addedThe
DOMSerializerimport is necessary for the improved serialization implementation.
21-35: Improved serialization handling for imagesThe updated serialization method now properly handles images by:
- Detecting if images are present in the content
- Only returning an empty string if there's no text content AND no images
- Using ProseMirror's
DOMSerializerto properly serialize all content including imagesThis is a significant improvement over the previous implementation which might not have correctly handled image nodes.
src/translations/sv.ts (1)
37-43: Comprehensive accessibility translations addedThe added translation strings provide helpful feedback for screen reader users during image operations:
- Upload in progress state
- Upload failure state
- Upload success state
- Instructions for using the resize handle
These translations enhance the accessibility of the image handling features, which is important for users relying on assistive technologies.
src/translations/en.ts (2)
37-43: Comprehensive accessibility translations addedThe added translation strings provide helpful feedback for screen reader users during image operations:
- Upload in progress state
- Upload failure state
- Upload success state
- Instructions for using the resize handle
These translations enhance the accessibility of the image handling features, which is important for users relying on assistive technologies.
37-43:❓ Verification inconclusive
Verify translations are consistent with Swedish version
The English translations appear to convey the same meaning as the Swedish translations, but with slight differences in phrasing. For example, the success message in English includes "successfully" while the Swedish version doesn't have this emphasis.
🏁 Script executed:
#!/bin/bash # Compare the structure of English and Swedish translation files to ensure they have the same keys echo "English translation keys:" grep -o "'editor-image-view[^']*'" src/translations/en.ts | sort echo "Swedish translation keys:" grep -o "'editor-image-view[^']*'" src/translations/sv.ts | sortLength of output: 595
Translation Keys Consistency Check Complete
- The English and Swedish translation files both include the same four keys, which confirms that the file structures are consistent.
- The only noted difference is that the English success message includes “successfully,” whereas the Swedish version omits this extra emphasis.
- Please verify that this slight difference in phrasing is intentional and aligns with the overall tone and messaging guidelines for your application.
src/components/text-editor/utils/markdown-converter.ts (1)
11-47: Integrate image node serialization carefullyGreat addition of the image node serializer via
getImageNodeMarkdownSerializer. This approach cleanly extends the default ProseMirror markdown serializer to cover image nodes. Make sure you have corresponding unit tests confirming that image nodes are serialized as expected and that all attributes (e.g.,alt,src) appear correctly in the output.🧰 Tools
🪛 Biome (1.9.4)
[error] 23-27: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (2)
10-20: Plugin initialization looks goodThe
createImageViewPluginfunction correctly instantiates a ProseMirror plugin with a custom node view for images. This ensures a clean, modular design. No immediate concerns here.
127-162: States managed well with ARIA labelsThe transitions to loading, success, and failed states are well-defined, and the removal of spinners or handles prior to each state is clean. The ARIA labeling here is thorough, providing clear feedback for assistive technologies. No issues to flag in this block.
src/components/text-editor/prosemirror-adapter/plugins/image/view.scss (1)
65-71:✅ Verification successful
Check browser support for the
:has()pseudo-classThe
:has()selector is powerful but can lack comprehensive support in older browser versions. It is supported in many modern browsers but may still pose compatibility issues. Confirm your minimum browser requirements or provide fallbacks if necessary.Also applies to: 107-113
🌐 Web query:
How widely supported is the CSS :has() pseudo-class in modern browsers, including Safari?💡 Result:
The CSS
:has()pseudo-class, which allows selecting parent elements based on their children, is now widely supported across all major browsers as of 2024:Browser Support Overview
- Safari: Supported since 15.4 (released March 2022) [2][5][6][9]
- Chrome/Edge: Supported since 105 (August 2022) [5][9]
- Firefox: Supported since 121 (December 2023) [1][9][10]
- Opera: Supported since 91 [10]
Key Details for Safari
- Full support in Safari 15.4+, covering all modern iOS/macOS devices [2][6][9]
- Works in Safari Technology Preview builds since December 2021 [6]
- Accounts for ~95% of Safari users on versions 15.4 and newer [2][9]
Global Compatibility
- Supported in 92% of browsers globally as of late 2024 [9]
- Considered a stable feature in CSS Selectors Level 4 specification [3][7]
- Can be used with progressive enhancement via
@supportschecks [3]:@supports (selector(:has(*))) { /* :has() styles */ }While legacy browsers (e.g., Safari <15.4, Firefox <121) lack support, the pseudo-class is now viable for production use in most modern web applications. Developers should pair it with feature detection for critical layouts [3][8].
Citations:
- 1: https://dev.to/yuridevat/css-has-pseudo-class-p6g
- 2: https://www.lambdatest.com/web-technologies/css-has-safari
- 3: https://blog.logrocket.com/advanced-guide-css-has-selector/
- 4: https://css-tricks.com/the-css-has-selector/
- 5: https://www.w3schools.com/cssref/sel_has.php
- 6: https://webkit.org/blog/13096/css-has-pseudo-class/
- 7: https://developer.mozilla.org/en-US/docs/Web/CSS/:has
- 8: https://www.sitepoint.com/css-is-where-has-pseudo-class-selectors/
- 9: https://www.joshwcomeau.com/css/has/
- 10: https://caniuse.com/css-has
Action: Confirm browser compatibility for
:has()pseudo-class usageBased on the latest data as of 2024, the CSS
:has()pseudo-class is now widely supported across major browsers:
- Safari: Supported since 15.4 (released March 2022)
- Chrome/Edge: Supported since 105 (August 2022)
- Firefox: Supported since 121 (December 2023)
- Opera: Supported since 91
If your application targets modern browsers, the code is fine as is. However, if you must support legacy browsers (e.g., Safari versions older than 15.4 or Firefox versions prior to 121), you may want to implement an
@supportsfeature check or provide fallback styles.Also applies to: lines 107-113
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (6)
41-42: Imports for image plugins look good.These plugin imports correctly extend the editor to handle image insertion and viewing.
48-52: New image-related types for the text editor.The added imports (
TriggerCharacter,ImageInserter,ImageInfo) are a solid approach to typed event handling for images.
160-167: Event emitter for imagePasted is well-defined.Exposing
imagePastedallows external components to handle inline image insertion workflows effectively.
169-176: Event emitter for imageRemoved is well-defined.Similarly,
imageRemovedhelps external components handle cleanup and external storage removal.
352-353: The image node extension is properly appended to the editor schema.This ensures the schema recognizes image nodes and supports inline image functionality.
389-394: Image plugins integrated successfully.Using
createImageInserterPluginandcreateImageViewPlugineffectively extends ProseMirror for image insertion and state management.src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (7)
22-56: Consider large file handling in paste flows.
If pasting large images is common, evaluate memory usage and performance impacts. You might add checks or transform large images before inserting them.
58-67: Efficient scanning approach.
getImagesFromTransactioneffectively gathers image nodes. This usage ofdescendants()is appropriate for typical ProseMirror documents.
69-87: Multiple removal events.
findAndHandleRemovedImagesdispatches an event for each removed image. Ensure duplicative calls toimageRemovedCallbackare expected, especially if multiple images are removed in one transaction.
89-100: Verify concurrency for multi-upload scenarios.
imageInserterFactorycreates multiple inserters. If users paste several images quickly, double-check that eachfileInfoIdremains unique, preventing collisions in doc states.
119-141: Avoid partial overwrites when re-calling the inserter.
WhencreateImageInserterreplaces the node, ensure repeated calls or partial updates don't cause conflicts if the image was already replaced or manipulated by other code.
143-165: Consistent naming for failure states.
createFailedThumbnailInsertersets the state toFAILED, aligning with theImageStateenum. This is clear and keeps the code coherent.
167-203: Validate file types more thoroughly.
Currently,file.type.startsWith('image/')is the detection check. If necessary for your scenario, consider stricter or additional validations (like MIME type checks).etc/lime-elements.api.md (3)
985-993: Simple and expressive image states.
FAILED,LOADING, andSUCCESSprovide excellent clarity on each phase of image handling. This enum usage is straightforward and consistent.
1680-1683: Consistent naming for new image events.
onImagePastedandonImageRemovedmaintain clarity and sync well with the rest of the components. Good alignment with standard naming conventions.
1805-1808: Compatibility withonImagePastedandonImageRemoved.
Mirroring these event handlers inlimel-text-editorensures uniform patterns acrosslimel-prosemirror-adapterand your editor components, simplifying usage for consumers.
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.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: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (2)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (2)
38-47: Add a unique key property when returning multiple elements.This helps Stencil (and similar frameworks) keep track of each element more reliably and avoid potential rendering issues.
return [ <limel-text-editor + key="text-editor" value={this.value} onChange={this.handleChange} onImagePasted={this.handleImagePasted} onImageRemoved={this.handleImageRemoved} />, <limel-checkbox + key="upload-fails-checkbox" label="Upload image fails - insert failed thumbnail" onChange={this.handleFailedThumbnailChange} />, ];🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
107-114: Remove or implement logic in the empty try block.Because there is no code inside the
trythat can throw, thecatchblock is unreachable. Either add deletion logic or remove thetry-catchstructure.- try { - // Delete image from external file storage. - } catch (error) { - console.error( - `Failed to delete image ${imageInfo.fileInfoId}`, - error, - ); - } + // Here you could implement the actual deletion logic or remove + // this try-catch altogether if no exceptions can be thrown.🧰 Tools
🪛 Biome (1.9.4)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
fe3d5d6 to
6fbc8a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (4)
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (1)
46-53: 🧹 Nitpick (assertive)Consider adding error handling and user feedback for large images.
This approach calls
insertThumbnail()followed byinsertImage()without error handling. Large images or unexpected failures might cause a sub-optimal user experience. Handling potential errors and providing user feedback can enhance robustness.private handleImagePasted = async ( event: LimelTextEditorCustomEvent<ImageInserter>, ) => { const imageInserter = event.detail; - imageInserter.insertThumbnail(); - imageInserter.insertImage(); + try { + imageInserter.insertThumbnail(); + imageInserter.insertImage(); + } catch (error) { + console.error('Failed to insert image:', error); + // Optionally, provide some UI feedback or fallback behavior here + } };src/components/text-editor/utils/markdown-converter.ts (1)
80-85: 🧹 Nitpick (assertive)Avoid relying on DOM-based checks for image detection.
Scanning
view.dom.innerHTMLto detect images can be fragile if the DOM structure changes. A more robust approach is to scanview.state.docfor an image node.Here’s an example of how you can locate image nodes in the ProseMirror document instead:
- const hasImage = view.dom.innerHTML.includes('<img'); + let hasImage = false; + view.state.doc.descendants((node: ProseMirrorNode) => { + if (node.type.name === 'image') { + hasImage = true; + } + });src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
185-206:⚠️ Potential issuePotential stale attributes in success state.
When both old and new node states are
SUCCESS, this logic prevents resettingsrcoralt. If the image source or alt text changes while staying inSUCCESS, it won’t update. Consider checking whether attributes differ even within the same success state to avoid stale data.Possible fix:
- if (!this.transitioningBetweenSuccessStates(node)) { + if ( + !this.transitioningBetweenSuccessStates(node) || + node.attrs.src !== this.node.attrs.src || + node.attrs.alt !== this.node.attrs.alt + ) { this.img.src = node.attrs.src; this.img.alt = node.attrs.alt; }src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
38-47: 🛠️ Refactor suggestion**Add unique key properties for elements returned in an array **
When returning React- or Stencil-like elements in an array, each element should have a unique
keyto prevent potential rendering issues:return [ - <limel-text-editor - value={this.value} - onChange={this.handleChange} - onImagePasted={this.handleImagePasted} - onImageRemoved={this.handleImageRemoved} - />, - <limel-checkbox - label="Upload image fails - insert failed thumbnail" - onChange={this.handleFailedThumbnailChange} - />, + <limel-text-editor + key="text-editor" + value={this.value} + onChange={this.handleChange} + onImagePasted={this.handleImagePasted} + onImageRemoved={this.handleImageRemoved} + />, + <limel-checkbox + key="upload-fails-checkbox" + label="Upload image fails - insert failed thumbnail" + onChange={this.handleFailedThumbnailChange} + />, ];🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
etc/lime-elements.api.md(3 hunks)src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx(1 hunks)src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.scss(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(5 hunks)src/components/text-editor/text-editor.tsx(5 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/html-converter.ts(2 hunks)src/components/text-editor/utils/markdown-converter.ts(3 hunks)src/translations/en.ts(1 hunks)src/translations/sv.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (37)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss (1)
172-172: Ensure consistency with SCSS imports.The referenced path
plugins/image/view.scsslooks correct and aligns with the new image-handling plugin. Just keep an eye on any future refactoring of directory structures to avoid broken imports.src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (2)
6-22: Documentation clarity is on point.Your explanation of the base64 encoding approach for inline images is concise and aligned with best practices. It's helpful that you highlight the recommendation to store images externally in production scenarios.
28-29: Good choice of default state value.Initializing
valuewith a placeholder text gives immediate clarity to the user. This is a user-friendly approach that helps demonstrate the image-paste feature out of the box.src/components/text-editor/text-editor.tsx (6)
6-11: Imports look correct.The newly imported types
TriggerCharacter,TriggerEventDetail,ImageInserter, andImageInfoappear appropriately utilized in this file. This organization improves clarity and type safety.
28-29: Good addition of new examples.Listing the newly introduced examples for inline images—both with external file storage and base64—helps guide consumers on different usage scenarios.
171-179: Private event naming.The
imagePastedevent is marked as@private, which helps signal its experimental usage. Ensure the Stencil build configuration and consumer usage consistently treat this event as internal or alpha, since@Event()typically emits a public event if used externally.
180-187: Event consistency and scope.Similarly,
imageRemovedis declared@private. If external consumers or examples rely on this event, you may wish to remove theprivatedoc tag or otherwise clarify the intended usage in the API docs.
268-269: Appropriate event bindings.Connecting
onImagePastedandonImageRemovedto the adapter is straightforward and aligns with the new plugin architecture. This integration nicely bridges the editor’s UI with the plugin’s events.
338-346: Internal event handlers are well-structured.Stopping event propagation and re-emitting via
imagePastedandimageRemovedis an effective approach to maintain a clean separation between the ProseMirror adapter and this component’s higher-level APIs.src/components/text-editor/utils/html-converter.ts (2)
5-5: Good import addition.Importing
DOMSerializerhere is appropriate for converting ProseMirror document fragments into DOM elements.
26-34: Solid serialization approach.Creating a container element, appending the serialized fragment, and returning
div.innerHTMLensures consistent handling of special nodes. This is a clean solution for capturing final HTML output.src/components/text-editor/text-editor.types.ts (3)
2-2: Import usage is clear.The
FileInfoimport is correctly referenced, confirming thatImageInserternow depends on shared file types.
94-101: Enum definition looks good.This enumeration cleanly represents the loading states of images.
103-120: Interface design is consistent.Properties for image identification, source, and state are sensibly organized and documented.
src/translations/sv.ts (1)
37-43:✅ Verification successful
Validate translation placeholders.
The Swedish translations for image states look correct. To ensure that
{ filename }is replaced properly at runtime, verify the usage in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Search for usage of the new translation keys and review placeholders rg 'editor-image-view\.aria-label\.' -A 5Length of output: 3962
Translation placeholders validated and working as expected.
- The Swedish translation strings in
src/translations/sv.tscorrectly contain the{ filename }placeholder.- The usage in
src/components/text-editor/prosemirror-adapter/plugins/image/view.tsshows that the translator function is called with an appropriate replacement object for{ filename }.- Both the Swedish and English translation files exhibit a consistent placeholder format.
src/components/text-editor/utils/markdown-converter.ts (2)
11-11: Great use of a dedicated serializer.Importing and integrating the
getImageNodeMarkdownSerializerfunction is a clean approach to extend the default Markdown serializer with image support.
47-47: Serializer merging looks solid.Merging image node serializers into the default ProseMirror Markdown serializer is straightforward and maintainable.
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (3)
10-20: Well-structured plugin creation.The
createImageViewPluginfunction effectively encapsulates the translator dependency, returning a plugin with a custom node view. This is a clean pattern for modularizing functionality.
22-58: Consistent ARIA usage in the constructor.The constructor properly sets
aria-liveandaria-busyattributes based on the image state, ensuring a good accessibility baseline. Continue verifying that screen readers accurately describe loading states.
140-153: Intuitive resize handles.Providing two resize handles improves usability for adjusting width from either edge. The ARIA attributes, including
role="slider", are well-used to convey handle behavior to assistive technologies.src/components/text-editor/prosemirror-adapter/plugins/image/view.scss (3)
14-17: **Usetransforminstead ofscalefor transitions **The
scaleproperty is not a standard CSS property, and may not be fully supported across browsers. Replace it withtransform, for example:- opacity 0.2s ease, - scale 0.6s ease; + opacity 0.2s ease, + transform 0.6s ease;
24-28: **Applytransform: scale(...)instead ofscale**Similarly,
scaleis not widely recognized in CSS. Usetransform: scale(...)for greater compatibility:- scale: 0.98; + transform: scale(0.98);
65-71:❓ Verification inconclusive
**
:has()pseudo-class has limited browser support **The
:has()selector is not fully supported in all browsers. Verify your target browser support or provide a graceful fallback for older browsers. If needed, please search the web for best practices and mention the tested browsers in your PR tests.
🌐 Web query:
Is ":has()" compatible with all major browsers?💡 Result:
The CSS
:has()selector is now supported in all major browsers as of late 2023, with full implementation across Chrome, Edge, Firefox, Safari, and Opera. Here's the latest browser compatibility breakdown:Browser Support Overview
Browser Minimum Supported Version Release Date Chrome 105+ August 2022 Edge 105+ August 2022 Firefox 121+ December 2023 Safari 15.4+ March 2022 Opera 91+ August 2022 Key Details
- Global Support: ~92% of users worldwide have access to
:has()as of September 2024[3][4].- Firefox Lag: Firefox was the last major browser to add support in December 2023[3][5].
- No IE11 Support: Internet Explorer and legacy browsers remain incompatible[2][4].
Feature Detection
For graceful degradation:
@supports (selector(:has(*))) { /* Modern styles using :has() */ } @supports not (selector(:has(*))) { /* Fallback styles */ }Performance Considerations
While widely supported, avoid overusing
:has()for:
- Complex DOM structures
- Chained conditions (e.g.,
.a:has(.b):has(.c))- Frequently updated dynamic content[1][2]
Major frameworks like React, Vue, and Tailwind CSS now support
:has()in their latest versions[1].Citations:
- 1: https://blog.logrocket.com/advanced-guide-css-has-selector/
- 2: https://www.lambdatest.com/blog/css-has-selector/
- 3: https://www.joshwcomeau.com/css/has/
- 4: https://caniuse.com/css-has
- 5: https://dev.to/yuridevat/css-has-pseudo-class-p6g
- 6: https://css-tricks.com/the-css-has-selector/
- 7: https://caniuse.com
- 8: https://ishadeed.com/article/css-has-guide/
Action: Confirm Browser Support for
:has()in the Image Plugin StylesThe CSS
:has()pseudo-class is now supported across all major browsers (Chrome 105+, Edge 105+, Opera 91+, Safari 15.4+, and Firefox 121+). However, legacy browsers such as Internet Explorer still lack support. Please confirm whether your project needs to support these older browsers. If legacy support is required, consider adding a graceful fallback with feature queries, for example:@supports not (selector(:has(*))) { /* Fallback styles for unsupported browsers */ }Ensure that your PR tests mention the browsers you’ve validated to avoid any unexpected behavior.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (2)
22-22: **Correct the grammar in the documentation **Change “Is the image upload fails” to “If the image upload fails” for clarity and correctness.
109-114: **Remove or repurpose the empty try block **The code in the
tryblock is empty, so thecatchblock is unreachable. Either remove the try-catch structure or add code that can actually throw an exception:- try { - // Delete image from external file storage. - } catch (error) { - console.error( - `Failed to delete image ${imageInfo.fileInfoId}`, - error, - ); - } + // Here you can implement actual deletion logic or remove this try-catch altogether + // Delete image from external file storage if needed🧰 Tools
🪛 Biome (1.9.4)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (4)
5-5: **Consider removing stale entries fromimageCache**The global
imageCachecan keep references to images, potentially causing memory bloat. Ensure that old or unused entries are removed if they’re no longer needed.
25-31: **Add descriptive placeholder text for LOADING or FAILED images **Currently,
<span></span>is used for images in loading or failed states, resulting in an empty placeholder. Consider adding short descriptive text (e.g., “(Image loading…)” or “(Image failed)”) to improve accessibility.
64-71: **Encourage descriptive alt text for accessibility **The default alt is set to an empty string, which can negatively impact accessibility. Consider prompting users to provide a meaningful alt description when possible.
80-89: **Invalidate or clear cache references for updated/removed images **When rendering images from
imageCache, ensure you remove or refresh the cache entries if an image is updated or removed, preventing stale data and reducing memory usage.src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (1)
22-56: Verify edge cases for removed images.
IncreateImageInserterPlugin, removed images are handled byfindAndHandleRemovedImages, which expects a stablefileInfoId. When images share the same attributes (possibly due to duplication), ensure no collisions occur.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (4)
41-42: Imports for new plugins look good.
These imports (createImageInserterPlugin,createImageViewPlugin) are correctly referenced. No concerns here.
54-54: Good use of the separategetImageNodeutility.
This explicit import clarifies how the schema is extended. No issues identified.
314-316: Avoid using{}as a type for function parameters.
This feedback has appeared before in past reviews. Use a more explicit type orRecord<string, unknown>for clearer contract.
389-394: Event emitters wired correctly into the plugin.
Passingthis.imagePasted.emitandthis.imageRemoved.emitas callbacks to thecreateImageInserterPluginis aligned with the plugin’s API. This integration looks solid.etc/lime-elements.api.md (3)
969-974: Document each property ofImageInfo.
This was raised in a previous review. More in-depth doc comments can help other developers integrate correctly.
976-983: Clarify the lifecycle ofImageInsertermethods.
As previously mentioned, providing usage examples or describing valid call sequences helps reduce confusion.
985-993: Enum states are clear.
FAILED,LOADING, andSUCCESSare well defined and self-explanatory. No concerns here.
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
6fbc8a1 to
916bec0
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (1)
46-53: 🧹 Nitpick (assertive)Consider adding error handling for image insertion.
The current implementation lacks error handling which could lead to a poor user experience if image insertion fails.
private handleImagePasted = async ( event: LimelTextEditorCustomEvent<ImageInserter>, ) => { const imageInserter = event.detail; - imageInserter.insertThumbnail(); - imageInserter.insertImage(); + try { + imageInserter.insertThumbnail(); + imageInserter.insertImage(); + } catch (error) { + console.error('Failed to insert image:', error); + // Consider providing user feedback about the failure + } };src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (1)
185-206:⚠️ Potential issueBug: attributes aren't updated when the image remains in success state.
Ifsrcoraltchanges whilestateis stillSUCCESS, the code inupdate()skips reassigning these attributes, resulting in stale data.To address this, compare them explicitly before skipping:
if ( !this.transitioningBetweenSuccessStates(newNode) || + newNode.attrs.src !== this.node.attrs.src || + newNode.attrs.alt !== this.node.attrs.alt ) { this.img.src = newNode.attrs.src; this.img.alt = newNode.attrs.alt; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
etc/lime-elements.api.md(3 hunks)src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx(1 hunks)src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.scss(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(5 hunks)src/components/text-editor/text-editor.tsx(5 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/html-converter.ts(2 hunks)src/components/text-editor/utils/markdown-converter.ts(3 hunks)src/translations/en.ts(1 hunks)src/translations/sv.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (57)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss (1)
172-172: LGTM! New styles import for image views.The addition of the image view styles import is properly placed at the end of the existing imports section, maintaining a logical organization of the stylesheet.
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (1)
18-21: Good documentation clarification about base64 usage.The documentation clearly explains that this approach is intended for simple examples, with appropriate recommendations for production use.
src/components/text-editor/text-editor.tsx (5)
6-11: LGTM! Appropriate imports for the new image functionality.The imports have been updated to include the necessary types for handling images.
28-29: Excellent documentation with example references.The documentation has been properly updated to reference the new example components for inline images.
171-188: Well-structured event definitions for image handling.The new events for
imagePastedandimageRemovedhave appropriate documentation, typing, and are correctly marked as private alpha features.
268-270: LGTM! Event handlers correctly wired.The new event handlers are properly connected to the ProseMirror adapter component.
338-346: Handlers follow established event handling pattern.The implementation of the image event handlers correctly follows the pattern used for other events in this file by stopping propagation and emitting the event detail.
src/translations/sv.ts (1)
37-43: Comprehensive accessibility translations for image states.The added translations provide excellent accessibility support for image operations with clear, descriptive messages for different states (loading, success, failure) and actions (resizing).
The placeholder
{ filename }is consistently used to provide context about which file is being processed, enhancing the user experience.src/components/text-editor/text-editor.types.ts (4)
2-2: Import looks good.
No issues found with this import statement.
66-92: Consider checks and guards to reduce state dependency.
Requiring the user to first callinsertThumbnailbefore callinginsertImageorinsertFailedThumbnailis error-prone. You might add some internal checks or refactor to reduce reliance on manual call ordering.
97-101: Enum usage is clear and concise.
DefiningImageStatewith explicit string values is a good approach for readability and type safety.
106-120: Interface definition looks solid.
ImageInfoprovides a straightforward structure for ID, source, and state. The naming is intuitive, and each property is well-documented.src/translations/en.ts (1)
37-43: Translation entries provide meaningful ARIA labels.
These new strings look good, and you are already using a fallback'file'internally if the filename is missing. This should prevent empty placeholders.src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (4)
6-7: Fixed minimum width is functional, but consider responsiveness.
A staticMIN_WIDTH = 10is acceptable for many use cases, but you might consider a more adaptive threshold for high DPI or mobile devices.
22-59: DOM manipulation is valid, but watch for performance.
Manually constructing and appending elements is efficient here, and the ARIA usage is commendable. Remain mindful of potential performance overhead if dealing with a large number of images.
74-80: Explore pointer events for cross-device compatibility.
This mouse-based resizing approach works on desktop browsers, but adopting Pointer Events could broaden support for touch and pen devices.
127-138: Loading state is well-implemented—consider a cancellable upload option.
Indeterminate progress and ARIA labels are good for accessibility. If uploads might take a while, adding a cancel or retry mechanism could enhance user experience.src/components/text-editor/utils/html-converter.ts (2)
5-5: Use ofNodealias is clear.
UsingNode as ProseMirrorNodehelps avoid confusion with the DOMNodetype. This import line looks good.
26-34: Neat usage ofDOMSerializer.
Wrapping the serialized fragment in a div and then returning itsinnerHTMLis a concise way to obtain HTML. Just be mindful that any top-level attributes or event listeners are dropped since we’re converting to bare HTML. This is acceptable for typical text-editor usage.src/components/text-editor/prosemirror-adapter/plugins/image/view.scss (10)
1-2: Sass mixin usage looks correct.
Importing shared mixins at the top helps keep styling consistent across the project.
13-17: Replace the experimentalscaleproperty withtransform: scale().
Thescaleproperty is not fully standardized and may have limited browser support. Usetransform: scale(...)for broader compatibility.- transition: - opacity 0.2s ease, - scale 0.6s ease; + transition: + opacity 0.2s ease, + transform 0.6s ease;
30-45: Pointer-events set to none on::before.
This ensures that hover or click events pass through to underlying elements, which can be beneficial for image interactions. This block looks solid.
47-56: Gray filter logic is straightforward.
Converting the image to grayscale helps visually indicate an error state. Just ensure there's a contrast mechanism for error states so it remains visible in all lighting conditions.
58-63: Hover styling is clear.
Increasing opacity on hover helps the user notice the interactive area more distinctly. This is an effective approach.
74-94: Sensible handle styling.
The sizing, elevation, and keyboard-focus visualization help ensure good accessibility for the resize handle.
95-104: Usetransform: translate(...)instead oftranslate:
Currently, the code usestranslate:syntax, which is not standard. Switch totransform: translate(...)for proper cross-browser compatibility.&.top-left { - translate: - -50%, - -50%; + transform: translate(-50%, -50%); } &.bottom-right { - translate: 50%, 50%; + transform: translate(50%, 50%); }
106-129: Gradual scaling on active states.
Applying a larger scale when grabbing or hovering helps the user identify the active resize handle. Continue to monitor user feedback on the scale factor and consider adjusting as needed.
130-142: Dedicated sizing for the smaller circle inside the handle.
This nested element approach is well-organized, ensuring the handle has a clear visual center. Looks good.
65-71:❓ Verification inconclusive
Verify support of
:has()selector for your target browsers.
The:has()pseudo-class is powerful but still has limited support in some older browsers. Confirm that it meets your compatibility requirements or provide a fallback.
🏁 Script executed:
#!/bin/bash # Description: Check if the :has() pseudo-class is safe to use. # We'll look for official references and known issues with :has(). # (No local repository files needed for this check, so skip cloning.)Length of output: 0
Action: Verify Browser Compatibility for the
:has()Pseudo-classThe use of
:has()can indeed present compatibility issues on older browsers. Since the shell script produced no output—and thus did not confirm support—we need to ensure that this selector is safe for our target browsers or that an appropriate fallback is provided. Please verify manually that the browsers in your support matrix (especially older versions or those like Firefox where:has()support is still lacking) are compatible with this feature.
- Next Steps:
- Confirm browser compatibility for
:has()via tools like MDN or caniuse.com.- If any target browser does not support
:has(), consider adding a fallback.src/components/text-editor/utils/markdown-converter.ts (3)
11-11: Seamless addition of the image serializer.
ImportinggetImageNodeMarkdownSerializerextends your existing markdown serialization for image nodes without impacting other logic.
47-47: AddinggetImageNodeMarkdownSerializerto the serializer.
Combining it with...defaultMarkdownSerializer.nodesensures images are properly handled. This is a clean extension.
84-85: Good usage ofmarkdownSerializer.
Returning the serializer’s output for non-empty content or if images are present maintains consistency with ProseMirror’s node structure.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (4)
22-22: Correct the grammar in the JSDoc comment.
38-43: Add a unique key to the text editor element.🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
44-47: Add a unique key to the checkbox element.🧰 Tools
🪛 Biome (1.9.4)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
107-114: Remove or implement logic in the empty try block.🧰 Tools
🪛 Biome (1.9.4)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (6)
1-11: Prefer alias imports or re-exports for readability.
14-17: Add JSDoc documentation for callbacks.
58-67: Check for unknown node types when searching descendants.
69-87: Consider performance for large documents.
102-118: Caution with large base64 images.
167-203: Validate the file size on paste.src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (4)
5-5: Manage potential memory growth in the image cache.
23-32: Provide placeholder text for LOADING or FAILED images in Markdown output.
60-105: Enhance alt text defaults for improved accessibility.
108-117: Remove or revalidate cache references when updating images.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (6)
41-42: Make sure to handle plugin ordering if necessary.
Although these import statements look fine as-is, sometimes the order in which plugins are applied impacts feature interactions. Ensure thatcreateImageInserterPluginandcreateImageViewPlugindon't have interdependencies requiring a specific order.
48-52: Prefer consistent import ordering.
This is similar to a past review comment about keeping your imports grouped or sorted for clarity.
54-54: Import ofgetImageNodelooks good.
This addition seamlessly extends the schema to handle image nodes.
160-167: Enhance documentation forimagePasted.
Consider providing usage examples for downstream consumers, such as how to handle thumbnail generation or async uploading after the image is pasted.
169-176: Enhance documentation forimageRemoved.
Clarify whether the consumer needs to remove associated resources (e.g., from storage) when receiving this event.
314-316: Replace{}with a more descriptive type.
Avoid using{}as a type for function parameters, as it matches any non-nullish value and can create unclear typing.etc/lime-elements.api.md (5)
969-974: Elaborate on theImageInfointerface.
Adding property-level doc comments fosters clarity for integrators.
976-983: Document the lifecycle of insertion methods inImageInserter.
Call out when and how each insertion method is expected to be used, particularly with regard to asynchronous workflows or error states.
985-993:ImageStateenum additions look solid.
This neatly categorizes the status of an image. If you anticipate more states (e.g.,pending), consider planning for them.
1680-1683: Add usage notes foronImagePastedandonImageRemovedin ProsemirrorAdapter.
Consider referencing an example usage scenario to guide developers in best practices.
1805-1808: Expose best practices for external integrators consumingonImagePastedandonImageRemoved.
Link to code examples or documentation demonstrating how to manage file storage or memory.
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
916bec0 to
923439e
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
etc/lime-elements.api.md(3 hunks)src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx(1 hunks)src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.scss(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(5 hunks)src/components/text-editor/text-editor.tsx(5 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/html-converter.ts(2 hunks)src/components/text-editor/utils/markdown-converter.ts(3 hunks)src/translations/en.ts(1 hunks)src/translations/sv.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (45)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss (1)
172-172: Include plugin SCSS file
This import statement properly references the additional styles for image view functionality. Please ensure the imported file exists and is recognized by the build pipeline.src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (5)
1-5: Imports look good
All required dependencies are correctly imported. No issues found.
23-29: Stencil component registration
The@Componentdecorator and configuration look standard. No concerns here.
31-40: Rendering the base text editor
This setup is straightforward and well-structured for a read-write scenario. No issues noted.
42-44: Clean change handler
Your event handler correctly updates component state and is typical for Stencil pattern usage.
46-53: Consider adding error handling for large images
This repeats a prior recommendation: if large images are pasted, base64 encoding might be slow or could fail. Add try/catch or asynchronous feedback to the user.Here is a potential snippet to illustrate this approach:
private handleImagePasted = async ( event: LimelTextEditorCustomEvent<ImageInserter>, ) => { const imageInserter = event.detail; - imageInserter.insertThumbnail(); - imageInserter.insertImage(); + try { + imageInserter.insertThumbnail(); + // Provide feedback or loading indicators if needed + imageInserter.insertImage(); + } catch (error) { + console.error('Failed to insert image:', error); + // Optionally insert a "failed" indicator or message + } };src/components/text-editor/text-editor.tsx (3)
6-11: Relevant imports for image management
ImportingImageInserterandImageInfois correctly placed to enable extended image handling features.
28-29: New example components
Listing the file-storage and base64 examples is helpful for demonstrating multiple image handling approaches.
268-269: New event bindings
BindingonImagePastedandonImageRemovedin the editor adapter is correct. These handlers propagate editor-level events as Stencil events.src/translations/sv.ts (1)
37-43: Translations look good!These new aria-label entries appear to be well-formed for accessibility in Swedish. Ensure that placeholders like
{ filename }are properly replaced at runtime, and consider verifying that the translations make sense in all contexts where they are used.src/components/text-editor/utils/html-converter.ts (1)
5-6: Imports align with new serialization logic.No issues with importing
DOMSerializerandhasImageNode. This should help streamline the check for images in the document.src/components/text-editor/text-editor.types.ts (2)
2-2: New import is consistent with usage.Importing
FileInfois valid and aligns with the newImageInserterinterface.
66-92: Guard against dependency on prior calls.Requiring
insertThumbnailbeforeinsertImageorinsertFailedThumbnailmay lead to accidental misuse. Consider checks or a more robust design to prevent undefined thumbnail references.src/translations/en.ts (1)
37-43: Good use of placeholders with fallback behavior.You already address missing filenames in the NodeView code via
this.node.attrs.alt || 'file'. The translation strings themselves are concise and user-friendly.src/components/text-editor/utils/markdown-converter.ts (4)
11-14: Great approach importing dedicated image-handling utilities.Importing
getImageNodeMarkdownSerializerandhasImageNodecleanly centralizes image-related logic and helps keep the converter code organized.
50-50: Neat integration of the image node serializer.Merging
...getImageNodeMarkdownSerializer()intonodesensures proper markdown serialization of images.
83-86: Covers the edge case for empty text with images.By checking both trimmed text and
hasImageNode(view.state.doc), you avoid returning an empty string when the editor only contains images.
89-90: Clear return strategy for serialized content.Returning the document serialization unconditionally after ensuring text or images are present is a logical approach.
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (3)
6-6: Setting a minimal width to 10px is reasonable.This baseline is acceptable for desktop usage. If needed, you can dynamically adjust this for high-DPI or mobile scenarios in the future.
127-134: Good fallback logic for missing filenames.Using
filename: this.node.attrs.alt || 'file'ensures the user never sees an empty or undefined placeholder. Nicely done.
185-200: Updating src/alt when remaining in success state.Reusing the NodeView is great for performance, but if both the old and new node are in
ImageState.SUCCESS, attributes likesrcandaltare not updated, causing potential stale data. Consider comparing the old and new attributes before skipping the update block.Apply this diff to update attributes even if the state remains SUCCESS when they change:
if ( !this.transitioningBetweenSuccessStates(node) || + node.attrs.src !== this.node.attrs.src || + node.attrs.alt !== this.node.attrs.alt ) { this.img.src = node.attrs.src; this.img.alt = node.attrs.alt; }src/components/text-editor/prosemirror-adapter/plugins/image/view.scss (4)
3-12: Ensure the image wrapper respects container overflow.Using
inline-flexwithposition: relativecan lead to images exceeding their container boundaries if not explicitly constrained. For better responsiveness, consider adding rules likemax-width: 100%or other suitable constraints.
14-17: Replace the experimentalscaleproperty withtransform: scale().The
scaleshorthand is not yet well-supported across all browsers. Usetransform: scale(...)for better cross-browser compatibility.
25-26: Replace the experimentalscaleproperty withtransform: scale().Similarly,
scale: 0.98;is not standard CSS. Switch totransform: scale(0.98);.
92-104: Usetransform: translate(...)instead oftranslate:.Properties like
translate: -50%, -50%;andtranslate: 50%, 50%;are not valid standard CSS. Replace them withtransform: translate(-50%, -50%)ortransform: translate(50%, 50%).src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (2)
38-47: Add a unique key to each element in the returned array.When returning multiple elements, frameworks like React or Stencil need a
keyattribute to properly track each element’s identity. Include uniquekeyprops to avoid warnings and unexpected rendering behavior.🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
107-114: Remove or implement logic in the empty try block.This
tryblock has no statements that can throw an exception, so thecatchblock is never triggered. Either remove thetry-catchor add real error-prone logic inside thetryblock.🧰 Tools
🪛 Biome (1.9.4)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (6)
1-3: Consider grouping imports by functionality.Organizing your standard library imports, external library imports, and local imports into distinct sections can improve readability.
14-17: Add JSDoc documentation for callbacks.Documenting parameters and return types for
ImagePastedCallbackandImageRemovedCallbackmakes their usage more transparent to other developers.
58-67: Check for unknown node types when searching descendants.Relying solely on
node.type.name === 'image'might miss custom or extended image-like nodes. Consider a more flexible approach if your schema supports additional node types.
69-87: Consider performance for large documents.
findAndHandleRemovedImagesiterates over all tracked images. For very large documents, performance might degrade. If necessary, you could optimize checks to affected ranges only.
102-118: Caution with large base64 images.Inserting large base64 data directly into the document can bloat its size and hurt performance. Consider a more memory-friendly approach if very large images are expected.
167-203: Validate the file size on paste.Currently, all pasted images are accepted. If there are size limits or user experience concerns, add file size checks and user feedback.
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (4)
5-5: Manage potential memory growth in the image cache.This global
imageCachemight accumulate references to images. Ensure that outdated entries are removed to avoid excessive memory usage.
48-50: Provide placeholder text for loading or failed images.Currently,
<span></span>is emitted for images in the LOADING or FAILED state, which can lead to empty placeholders. Consider adding descriptive text like “(Image loading…)” or “(Image failed)”.
84-90: Enhance alt text defaults for improved accessibility.The node spec sets the
altattribute to an empty string by default, potentially reducing accessibility if it's never updated to something descriptive.
127-136: Remove or revalidate cache references when updating images.When using
imageCache.get(node.attrs.fileInfoId), ensure that outdated references are eventually cleared. This will help manage memory usage over time.etc/lime-elements.api.md (4)
970-974: Add or expand documentation forImageInfo.Explicit doc comments on each property can help clarify usage and reduce potential misinterpretations.
976-983: Clarify lifecycle of these insertion methods.For
ImageInserter, adding short descriptions about call sequences and side-effects helps developers integrate with the text editor more confidently.
1680-1683: Add usage notes for the new image-related event callbacks.
onImagePastedandonImageRemovedmight benefit from brief doc guidance, indicating whether additional post-processing is recommended.
1805-1808: Expose handling instructions for external integrators.Providing a usage example for
onImagePastedandonImageRemovedin the docs helps integrators see typical paste or removal flows.src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (4)
48-52: Prefer consistent import ordering.Keep import style consistent across your files. Consider placing
TriggerCharacter,ImageInserter, andImageInfoalongside other local type imports.
160-177: Add doc details for the new image events.
@Event()declarations forimagePastedandimageRemovedare useful. Clarifying how a consumer should respond to them can improve the developer experience.
314-316: Avoid using{}as a type for function parameters.
{}matches any non-nullish value. Specify a stricter type or useRecord<string, any>to make the parameter intent clearer.- private translator = (key: string, params = {}): string => { + private translator = (key: string, params: Record<string, unknown> = {}): string => {
389-393: Double-check error handling for image insertion.Before emitting
imagePastedorimageRemoved, ensure the image data is valid or not already handled, to avoid duplicate or null events.
src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
923439e to
8646ee1
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
36-49: 🛠️ Refactor suggestionProvide a unique key property for each element in the returned array
When returning arrays of elements, most virtual-DOM-based frameworks require a unique key to track changes.Here’s a suggested fix:
return [ - <limel-text-editor - value={this.value} - onChange={this.handleChange} - onImagePasted={this.handleImagePasted} - onImageRemoved={this.handleImageRemoved} - />, - <limel-checkbox - label="Upload image fails - insert failed thumbnail" - onChange={this.handleFailedThumbnailChange} - />, + <limel-text-editor + key="text-editor" + value={this.value} + onChange={this.handleChange} + onImagePasted={this.handleImagePasted} + onImageRemoved={this.handleImageRemoved} + />, + <limel-checkbox + key="upload-image-fails" + label="Upload image fails - insert failed thumbnail" + onChange={this.handleFailedThumbnailChange} + />, ];🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
etc/lime-elements.api.md(3 hunks)src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx(1 hunks)src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts(0 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/node.ts(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.scss(1 hunks)src/components/text-editor/prosemirror-adapter/plugins/image/view.ts(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss(1 hunks)src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx(5 hunks)src/components/text-editor/text-editor.tsx(5 hunks)src/components/text-editor/text-editor.types.ts(2 hunks)src/components/text-editor/utils/html-converter.ts(2 hunks)src/components/text-editor/utils/markdown-converter.ts(3 hunks)src/translations/en.ts(1 hunks)src/translations/sv.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/text-editor/prosemirror-adapter/plugins/image-remover-plugin.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (60)
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.scss (1)
172-172: Ensure the newly imported styles are integrated in the build
Great to see the dedicated SCSS import. Verify that your build pipeline includes theplugins/image/view.scssfile so these styles are applied correctly in all environments.src/components/text-editor/examples/text-editor-with-inline-images-base64.tsx (7)
1-5: Imports look good
All required imports seem appropriate. No issues found.
6-22: Documentation is clear and informative
You have provided a detailed JSDoc block explaining how the pasted images are handled. This helps clarify usage for other developers.
23-26: Well-structured component annotation
The@Componentconfiguration is consistent with Stencil best practices.
28-29: State initialization
Thanks for omitting the trivial type annotation; the compiler infers the type accurately. This is concise and clear.
31-40: Render method
Your usage of<limel-text-editor>with the relevant events and props is consistent and straightforward.
42-44: Simple change handler
ThehandleChangemethod properly updates the local state. Implementation looks good.
46-53: Consider adding error handling
This code immediately inserts both the thumbnail and the final image, but lacks error handling or user feedback if insertion fails.src/components/text-editor/text-editor.tsx (6)
6-11: New imports are well-structured
ImportingImageInserterandImageInfocentralizes image-related types for clarity.
28-29: Example references
Highlighting your inline-image examples in the doc block is helpful for new users exploring these features.
171-179: Private event visibility
DeclaringimagePastedasprivatemight limit external consumers. If your goal is to allow custom handling outside the component, consider making it public.
180-188: Image removal event
Similarly, if external clients need to react to image removal, consider adjusting the event’s access modifier.
268-269: Attachment of new event handlers
Good to see the new event handlers bound to the adapter. This ensures consistent propagation of image paste/remove events.
337-346: StopPropagation and re-emission
Forwarding events after stopping propagation is a clean approach. If you plan to handle errors in the future, inject any needed logic before re-emitting the event.src/translations/sv.ts (1)
37-43: Translations look good!
The placeholders and language usage appear correct and consistent with Swedish syntax. Great job ensuring accessibility by including aria-labels.src/components/text-editor/utils/html-converter.ts (1)
5-6:✅ Verification successful
Verify that the required dependencies are present.
These new imports look fine, but please confirm that theprosemirror-modelpackage is properly installed and thathasImageNodeis exported as expected.
🏁 Script executed:
#!/bin/bash # Check for `prosemirror-model` dependency in package.json fd package.json | xargs rg '"prosemirror-model"'Length of output: 123
Verification Complete: All dependencies and exports confirmed
- The
prosemirror-modeldependency is correctly specified in package.json (version ">=1.22.1").- The file at
src/components/prosemirror-adapter/plugins/image/nodeproperly exportshasImageNode.src/components/text-editor/text-editor.types.ts (3)
2-2: New import is fine.
No issues with referencingFileInfohere.
66-92: Guard the calling sequence for image insertion methods.
CallinginsertThumbnailbeforeinsertImageorinsertFailedThumbnailintroduces state dependency. Consider adding guards or a single coordinated method to reduce misuse.
94-101: Expand future states if needed.
Adding a queued or canceled state might help gracefully handle more use cases. The current three states (loading, failed, success) are a good start.src/translations/en.ts (1)
37-43: Ensure fallback behavior for missing filename.As previously noted, if
filenameis missing or empty, the placeholders will be displayed as blank. Consider providing a fallback string inside the formatter to handle an undefined or empty filename gracefully.src/components/text-editor/utils/markdown-converter.ts (2)
11-14: Imports look solid.These imports properly centralize the image node handling logic and keep the file organized.
50-50: Good integration of the image node serializer.Invoking
getImageNodeMarkdownSerializer()helps ensure proper markdown conversion for images without cluttering the main serializer logic.src/components/text-editor/prosemirror-adapter/plugins/image/view.ts (8)
6-6: Consider higher or dynamic minimum width for broader device support.A
MIN_WIDTHof 10px is simple and effective on desktop, but might limit flexibility on higher DPI mobiles or tablets. A responsive approach or retrieving the minimum size from configuration could help.
10-20: Plugin creation is straightforward and modular.Using
nodeViewsin a dedicated plugin is clean and keeps the image logic encapsulated, simplifying future maintenance or extension.
22-58: Constructor’s accessibility setup looks good.Setting
aria-live="polite"and togglingaria-busybased on state is excellent for screen readers. Good job also appending the image directly in the constructor and handling dynamic attributes likesrcandalt.
127-138: Loading state is clearly communicated.Indicating a busy state via
aria-labeland including a linear progress component is user-friendly and accessible. Nicely done.
140-153: Success state provides convenient resize handles.Having handles at two corners simplifies resizing in both directions. This approach is intuitive for desktop users, and the ARIA attributes make it accessible.
164-183: Clean-up of obsolete elements is neat.Removing all child nodes except the image itself prevents orphaned spinners or handles when transitioning states. This keeps the DOM tidy.
185-190: Smart check for uninterrupted success states.This helper function ensures minimal flicker by confirming the node is already in
SUCCESS. It’s a good defensive measure for a fluid user experience.
192-206: Consider always updating attributes in success state.As previously noted, if
srcoraltchanges while both the old and new nodes are inSUCCESS, those updates might be skipped. Compare attributes for changes:if ( - !this.transitioningBetweenSuccessStates(node) + !this.transitioningBetweenSuccessStates(node) || + node.attrs.src !== this.node.attrs.src || + node.attrs.alt !== this.node.attrs.alt ) { this.img.src = node.attrs.src; this.img.alt = node.attrs.alt; }This ensures stale data is not retained when updating an already successful image.
src/components/text-editor/prosemirror-adapter/plugins/image/view.scss (6)
3-12: Ensure image wrapper respects container overflow
This suggestion was already covered in a past review comment, so marking it as duplicate.
14-17: Replace experimentalscaletransition property withtransform: scale(...)
This suggestion was already mentioned in a past review; using thescaleCSS property may result in inconsistent cross-browser support.
19-28: Address low opacity andscaleusage for accessibility
A past review has flagged that reducing opacity to0.3can impact visibility for some users with visual impairments, and thescaleproperty also has limited browser support.
92-98: Usetransform: translate(...)instead oftranslate:
Usingtranslate: -50%, -50%is non-standard. This has been discussed in previous feedback.
100-104: Usetransform: translate(...)instead oftranslate:
Same issue as above, flagged in a past review.
121-129: Replace thescaleproperty withtransform: scale(...)
This was highlighted in a previous review, asscale: 1.4;has limited browser support.src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (1)
107-114: Remove or implement logic in the empty try block
A past review already raised that this try-catch is effectively unreachable since thetryblock has no throwing code.🧰 Tools
🪛 Biome (1.9.4)
[error] 109-114: This code is unreachable
(lint/correctness/noUnreachable)
src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts (6)
1-11: Group or alias imports for improved readability
This was suggested in a past review to keep the imports more organized or consolidated via alias re-exports.
14-17: Add JSDoc for callback types
A previous review recommended documenting the arguments and return values forImagePastedCallbackandImageRemovedCallback.
58-67: Consider handling unknown or custom image-like nodes
A past review already flagged that checking onlynode.type.name === 'image'might skip extended node types.
69-87: Optimize performance with large documents
This concern was previously raised; consider more efficient ways to track or limit checks for removed images if the document is very large.
102-118: Avoid inserting large base64 images directly
This was mentioned in an earlier review; for very large images, consider an alternative approach to reduce memory usage during insertion.
167-203: Validate file size on paste
A past review suggested requiring size checks or feedback for large pasted images to prevent performance issues.src/components/text-editor/prosemirror-adapter/plugins/image/node.ts (4)
5-5: Revisit the global image cache to prevent memory leaks.This replicates a previous recommendation to manage entries in
imageCache. If image references become stale (e.g., an image is removed from the document), those entries can remain in the cache indefinitely. Ensure entries are either removed or overwritten to avoid memory growth over time.
44-51: Add descriptive placeholders for LOADING or FAILED states.This reiterates a prior comment suggesting that using
<span></span>for LOADING or FAILED images might be confusing or inaccessible to end-users. Consider including a text hint (e.g., “(Image loading…)” or “(Image failed)”) to convey the image’s status.
84-85: Encourage meaningful alt text to improve accessibility.By defaulting
altto empty, you risk reduced accessibility for assistive technologies. As previously advised, consider requiring or guiding developers to supply a descriptivealtwhen possible.
127-136: Remove or revalidate cached references upon image update.This is the same suggestion as before: when updating the existing image element, verify that any outdated references in
imageCacheare handled appropriately to avoid caching old, unused data.etc/lime-elements.api.md (5)
969-974: Add or expand documentation forImageInfo.As previously requested, consider clarifying how each field in
ImageInfois used (e.g., whetherfileInfoIdties to an internal file record, etc.) so integrators know how to populate or interpret these values.
976-983: Clarify the lifecycle and usage of methods inImageInserter.Reiterate the prior suggestion to document the expected sequence of calls and any side effects (e.g., does
insertImageassume the user has posted the file to a server?). This context helps developers integrate more confidently.
985-993: Improve enum documentation forImageState.As previously noted, add short doc descriptions for each value (
FAILED,LOADING, andSUCCESS) to help consumers understand when and why each state is assigned.
1680-1683: Provide usage notes foronImagePastedandonImageRemovedinlimel-prosemirror-adapter.Consumers will benefit from minimal guidance on when these events fire and how best to handle them, e.g., uploading the file in
onImagePastedor performing cleanup inonImageRemoved.
1805-1808: Expose image event handling instructions inlimel-text-editorfor external integrators.As with the other event interfaces, consider linking to usage examples or describing typical user flows (like hooking into file upload or showing a progress indicator).
src/components/text-editor/prosemirror-adapter/prosemirror-adapter.tsx (8)
41-42: Maintain consistent import ordering.This echoes the prior feedback about arranging local imports coherently, which helps keep the codebase tidy. Consider grouping related imports or re-exporting from a single index if that's part of your style guide.
48-52: Align local type imports with existing import conventions.Following up on earlier recommendations, place
TriggerCharacter,ImageInserter, andImageInfoconsistently among other local or external imports. This preserves readability and clarity.
54-54: Importing the image node logic looks good.The direct import of
getImageNodealigns well with the new image handling features.
160-168: Document image event usage more explicitly.Mirroring past feedback, expand these doc comments for
imagePasted, suggesting common use-cases (e.g., hooking to an upload API or generating thumbnails) so consumers can better implement the workflow.
170-176: Add clarity to theimageRemovedevent flow.Re-state prior guidance to clarify typical removal lifecycles—if integrators need to delete server-side files or update internal references, direct them on best practices.
314-316: Prefer typing over{}to reduce ambiguity.As discussed before, using
Record<string, unknown>or a more specific type forparamsclarifies its intended structure and avoids “any non-nullable object” pitfalls.
352-352: Appending the image node specification is effective.This properly integrates the image schema into the editor. Great addition!
389-393: Validate image data before emitting paste/remove events.This reaffirms earlier requests to ensure the image data is valid and not already processed. Failing to check might yield duplicate or erroneous events, so you might insert a quick guard clause here.
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
src/components/text-editor/prosemirror-adapter/plugins/image/view.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/node.ts:5-5
Timestamp: 2025-03-04T12:45:10.108Z
Learning: Images stored in caches within the text editor component need to have proper cleanup mechanisms to avoid memory leaks. Images can consume megabytes of memory each, making it especially important to remove them from caches when they're no longer needed in the editor.
🪛 Biome (1.9.4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (4)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (4)
1-8: Imports look good and appropriate for the functionality.The component imports the necessary dependencies from both Stencil Core and Lime Elements.
9-24: The documentation is clear and comprehensive.The JSDoc provides excellent documentation on how to handle inline images with external file storage, explaining the event flow and available methods.
37-48: Add key properties to array elements or use a Host wrapper.When rendering an array of elements, each element should have a unique key property to help frameworks track element identity and optimize rendering.
-return [ - <limel-text-editor - value={this.value} - onChange={this.handleChange} - onImagePasted={this.handleImagePasted} - onImageRemoved={this.handleImageRemoved} - />, - <limel-checkbox - label="Upload image fails - insert failed thumbnail" - onChange={this.handleFailedThumbnailChange} - />, -]; +return ( + <Host> + <limel-text-editor + value={this.value} + onChange={this.handleChange} + onImagePasted={this.handleImagePasted} + onImageRemoved={this.handleImageRemoved} + /> + <limel-checkbox + label="Upload image fails - insert failed thumbnail" + onChange={this.handleFailedThumbnailChange} + /> + </Host> +);Alternatively, if you prefer to keep the array return format:
return [ - <limel-text-editor + <limel-text-editor + key="text-editor" value={this.value} onChange={this.handleChange} onImagePasted={this.handleImagePasted} onImageRemoved={this.handleImageRemoved} />, - <limel-checkbox + <limel-checkbox + key="upload-fails-checkbox" label="Upload image fails - insert failed thumbnail" onChange={this.handleFailedThumbnailChange} />, ];🧰 Tools
🪛 Biome (1.9.4)
[error] 38-43: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
[error] 44-47: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
61-74: The image pasting handler looks well-implemented.The
handleImagePastedmethod appropriately:
- Inserts a thumbnail immediately
- Attempts to upload the image
- Either inserts the full image or a failure indicator based on the upload result
This gives users immediate feedback while handling the asynchronous upload process.
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: adrianschmidt
PR: Lundalogik/lime-elements#3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/node.ts:5-5
Timestamp: 2025-03-04T12:45:10.108Z
Learning: Images stored in caches within the text editor component need to have proper cleanup mechanisms to avoid memory leaks. Images can consume megabytes of memory each, making it especially important to remove them from caches when they're no longer needed in the editor.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (6)
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx (6)
1-8: LGTM! Appropriate imports for a Stencil component.The component imports all the necessary elements from '@stencil/core' and '@limetech/lime-elements' to handle both the component lifecycle and the image-related functionality.
9-24: Correct the grammar in the JSDoc comment.A small grammatical error in the JSDoc documentation.
- * Is the image upload fails, you can insert a failed thumbnail using the + * If the image upload fails, you can insert a failed thumbnail using the
25-34: LGTM! Clean component definition with proper state management.The component is properly defined with the right decorators and state variables. Good job removing the trivial type annotations as suggested in previous reviews.
36-51: LGTM! Well-structured render method using Host wrapper.The component correctly uses the Host element to wrap multiple child elements, as suggested in previous reviews. The event handlers are properly bound to the component methods.
53-76: LGTM! Well-implemented event handlers for image paste workflow.The event handling logic is well-structured, with a clear flow for image pasting:
- Insert a temporary thumbnail
- Attempt to upload the image
- Replace with the final image or a failure indicator based on upload result
This provides good user feedback during the potentially slow upload process.
78-101: Ensure the upload function returns a value in all code paths.The
uploadImagemethod doesn't explicitly return a value in the catch block, which means it will returnundefinedimplicitly when an error occurs. While thehandleImagePastedmethod handles this correctly by checkingif (imageSrc), it would be more explicit to returnnullin the catch block.private uploadImage = async (fileInfo: FileInfo): Promise<string> => { try { // Upload image to external file storage. // fileInfo.fileContent contains the image data. // Simulate upload delay. const imageSrc: string = await new Promise((resolve, reject) => { setTimeout(() => { if (this.uploadImageFails) { reject('Server error'); } else { resolve('https://cdn.lime-crm.com/mail-addin-logo.png'); } }, 2000); }); // Return the src url of the uploaded image. return imageSrc; } catch (error) { console.error( `Failed to upload image ${fileInfo.filename}: ${error}`, ); + return null; // Explicitly return null when upload fails } };
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Show resolved
Hide resolved
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Show resolved
Hide resolved
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Outdated
Show resolved
Hide resolved
adrianschmidt
force-pushed
the
inline-images-support
branch
from
c1d0709 to
5aae2b8
Compare
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
7503410 to
d7aae1c
Compare
adrianschmidt
force-pushed
the
inline-images-support
branch
2 times, most recently
from
5923411 to
483fe8f
Compare
There was a problem hiding this comment.
I just now actually tried this example for the first time, and holy smokes it's amazing! I mean, I knew exactly what I expected it to do, and it did exactly that, but it was still amazing to actually see! Sweet work! 🙌
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Outdated
Show resolved
Hide resolved
src/components/text-editor/examples/text-editor-with-inline-images-file-storage.tsx
Show resolved
Hide resolved
|
I've added some minor comments. Apart from that, this feels pretty ready to merge. I would perhaps like us to test that we don't have any unforeseen bugs when the image-functionality is not used. So we're not introducing bugs for existing consumers. |
adrianschmidt
force-pushed
the
inline-images-support
branch
from
b4a211b to
cdac798
Compare
FredrikWallstrom
force-pushed
the
inline-images-support
branch
2 times, most recently
from
53d12b7 to
4616349
Compare
src/components/text-editor/prosemirror-adapter/plugins/image/node.ts
Outdated
Show resolved
Hide resolved
FredrikWallstrom
force-pushed
the
inline-images-support
branch
from
7bda006 to
f91dfff
Compare
adrianschmidt
force-pushed
the
inline-images-support
branch
from
f91dfff to
1278215
Compare
|
🎉 This PR is included in version 38.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile:
Summary by CodeRabbit
New Features
Style