markdown editor #194
markdown editor #194
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/components/modules/messages/MessageUpdate/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
WalkthroughThis pull request introduces a comprehensive enhancement to the messaging system by integrating Markdown support across multiple components. The changes span the design system and messaging modules, adding new components like Changes
Sequence DiagramsequenceDiagram
participant User
participant SendMessage
participant MarkdownEditor
participant Markdown
participant MessagesList
User->>SendMessage: Compose message
SendMessage->>MarkdownEditor: Input markdown text
MarkdownEditor-->>SendMessage: Processed markdown
SendMessage->>MessagesList: Send message
MessagesList->>Markdown: Render message
Markdown-->>MessagesList: Formatted message
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
packages/design-system/components/Markdown/highlight.ts (2)
4-8: Global declaration of hljs
Defininghljson the globalwindowobject can lead to potential conflicts or shadowing in certain environments. Consider abstracting the highlight logic instead of adding it to the global scope.
14-16: Conditional attachment to the window object
While checkingtypeof window !== 'undefined'for SSR compatibility is helpful, you may still want to isolatehljsusage in a dedicated utility for improved testability and reduced global pollution.packages/design-system/components/images/LazyLoadImage/utils.ts (1)
3-15: Provide a fallback or error for unrecognized ratio.Currently, this function returns
undefinedif a non-predefined ratio is passed, which may break layouts if the calling code doesn't handleundefined. Consider providing a meaningful fallback or throwing an error for unrecognized ratios.Example refactor:
-export function getRatio(ratio = '1/1') { - return { - '4/3': 'calc(100% / 4 * 3)', - '3/4': 'calc(100% / 3 * 4)', - '6/4': 'calc(100% / 6 * 4)', - '4/6': 'calc(100% / 4 * 6)', - '16/9': 'calc(100% / 16 * 9)', - '9/16': 'calc(100% / 9 * 16)', - '21/9': 'calc(100% / 21 * 9)', - '9/21': 'calc(100% / 9 * 21)', - '1/1': '100%', - }[ratio] +export function getRatio(ratio = '1/1'): string { + const ratioMap: Record<string, string> = { + '4/3': 'calc(100% / 4 * 3)', + '3/4': 'calc(100% / 3 * 4)', + '6/4': 'calc(100% / 6 * 4)', + '4/6': 'calc(100% / 4 * 6)', + '16/9': 'calc(100% / 16 * 9)', + '9/16': 'calc(100% / 9 * 16)', + '21/9': 'calc(100% / 21 * 9)', + '9/21': 'calc(100% / 9 * 21)', + '1/1': '100%', + } + return ratioMap[ratio] || ratioMap['1/1'] }packages/design-system/components/images/LazyLoadImage/types.ts (1)
1-20: Unify ratio definitions with the logic in getRatio.Ensure that the string literals in
LazyLoadImageRatioalways match the explicit keys handled bygetRatioto avoid runtime mismatches. Consider centralizing these values in a single script or shared enum.packages/design-system/components/images/LazyLoadImage/index.tsx (2)
14-36: Provide clearer naming & doc-block for clarity on forwarded ref usage.While using
forwardRefhere is appropriate, adding a short JSDoc-style comment describing the forwarded ref purpose will help other developers understand how to interact with the component.+/** + * LazyLoadImage component uses forwardRef to pass a ref to the underlying container. + * This can be especially useful in scenarios where parent components need direct access + * to DOM elements for measurements or scroll events. + */ const LazyLoadImage = forwardRef<HTMLSpanElement, LazyLoadImageProps>( ... )
41-52: Overlay logic is implemented neatly; consider layering options to handle complex backgrounds.Everything looks organized. If you anticipate gradient overlays or additional styling, expand the overlay logic with more flexible props or a new prop interface (e.g.,
overlayProps).packages/components/modules/messages/SendMessage/index.tsx (1)
6-6: Check if dynamic imports are needed for performance.Importing
MarkdownEditoron demand could improve initial load times if large dependencies are involved. Consider using a dynamic import if performance metrics show the need for lazy loading.packages/design-system/components/Markdown/styled.ts (3)
16-20: Pause on styling<br>as a grid element.
Styling the<br>tag withdisplay: 'grid'and addingcontentcan break standard line-break semantics in certain contexts. Consider verifying it in cross-browser scenarios to ensure it doesn’t create unexpected spacing issues.
42-70: Check blockquote’s large font size for consistency.
AfontSizeof1.5emfor blockquotes may be significantly larger than the surrounding text, which can disrupt reading flow in some designs. Confirm the design intention or consider adjusting it if it looks out of place in the final UI.
106-140: Improve keyboard accessibility for custom checkboxes.
The custom styling for checkboxes is visually appealing, but ensure keyboard navigation, focus styling, and ARIA attributes remain intact. You may need additional:focusor:focus-visiblestyles for better accessibility compliance.packages/design-system/package.json (2)
54-54: Revisit the re-addition of@baseapp-frontend/tsconfig.
Removing and re-adding the same version might indicate a version mismatch or a prior installation issue. Double-check that this dev dependency is at the correct version to maintain consistent type definitions across all packages.
66-70: Ensure correct type definitions batch import.
The type definitions forhighlight.jsandreact-lazy-load-image-componentare pinned to specific versions. If those libraries update, these type definitions might lag behind. Keep an eye on new library releases to maintain compatibility.packages/design-system/components/inputs/SocialTextField/types.ts (1)
15-15: Consider a more robust type strategy to avoid potential mismatches.By using a union type for
FC<TextareaFieldProps> | FC<MarkdownEditorProps>, it’s possible to introduce runtime type mismatches if the parent incorrectly passes markdown-based components expecting text-area props, or vice versa. Consider introducing a safe fallback or a discriminated union to guarantee correct usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
packages/components/modules/messages/MessageUpdate/index.tsx(2 hunks)packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx(2 hunks)packages/components/modules/messages/SendMessage/index.tsx(2 hunks)packages/design-system/components/Markdown/highlight.ts(1 hunks)packages/design-system/components/Markdown/index.tsx(1 hunks)packages/design-system/components/Markdown/styled.ts(1 hunks)packages/design-system/components/Markdown/types.ts(1 hunks)packages/design-system/components/images/LazyLoadImage/__storybook__/stories.tsx(1 hunks)packages/design-system/components/images/LazyLoadImage/index.tsx(1 hunks)packages/design-system/components/images/LazyLoadImage/types.ts(1 hunks)packages/design-system/components/images/LazyLoadImage/utils.ts(1 hunks)packages/design-system/components/images/index.ts(1 hunks)packages/design-system/components/inputs/MarkdownEditor/MarkdownEditorStyles.css(1 hunks)packages/design-system/components/inputs/MarkdownEditor/index.tsx(1 hunks)packages/design-system/components/inputs/MarkdownEditor/types.ts(1 hunks)packages/design-system/components/inputs/SocialTextField/index.tsx(2 hunks)packages/design-system/components/inputs/SocialTextField/types.ts(1 hunks)packages/design-system/components/inputs/index.ts(1 hunks)packages/design-system/index.ts(1 hunks)packages/design-system/package.json(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/design-system/components/inputs/MarkdownEditor/MarkdownEditorStyles.css
🧰 Additional context used
🪛 GitHub Actions: Main Workflow
packages/design-system/components/inputs/SocialTextField/index.tsx
[error] 51-51: Type mismatch error: The provided type for component props is not assignable to the expected type. This appears to be related to Control and MDEditorProps type definitions.
packages/design-system/components/Markdown/index.tsx
[error] 2-2: CommonJS module imports will produce 'require' calls but referenced file is an ECMAScript module and cannot be imported with 'require'
🪛 Biome (1.9.4)
packages/design-system/components/Markdown/index.tsx
[error] 20-20: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (21)
packages/design-system/components/images/index.ts (1)
3-3: Add new export for LazyLoadImage
This addition looks consistent with the existing pattern of exporting image components in this index file.packages/design-system/components/Markdown/types.ts (1)
1-6: Interface extension approach looks good
By extendingOptionsfromreact-markdownand adding a flexiblesxprop, you keep styling concerns cleanly separated while retaining all base Markdown options.packages/design-system/components/inputs/MarkdownEditor/types.ts (1)
1-11: MarkdownEditorProps type structure
CombiningMDEditorPropsandFormControlalong with optional handlers (e.g.onKeyDown) and placeholders appears both flexible and consistent with common form patterns in this codebase.packages/design-system/components/Markdown/highlight.ts (2)
1-2: Importing Highlight.js and applying a theme
The chosentomorrow-night.csstheme ensures a dark code syntax highlight. This is a sound default for improved readability.
10-12: Whitelisted languages for config
Specifically configuring only necessary languages helps optimize performance while still offering robust highlighting. This targeted approach is good practice.packages/design-system/components/inputs/index.ts (1)
1-2: Re-exporting MarkdownEditor and types looks good.This approach provides a consistent entry point for consumers of the design-system, keeping import statements clean and well-organized.
packages/design-system/components/images/LazyLoadImage/__storybook__/stories.tsx (1)
1-28: Code structure and Storybook usage look great!All newly introduced lines in this Storybook file appear well-structured, with clear separation of the default story variant. The usage of Meta, StoryFn, and StoryObj from
@storybook/reactadheres to best practices, making it straightforward to add additional story variations in the future.packages/design-system/components/inputs/MarkdownEditor/index.tsx (1)
12-36: Integration with@uiw/react-md-editoris properly implemented.The Markdown editor setup is intuitive, especially the usage of
onKeyDownfor custom controls. The container class approach is concise, and the custom placeholder logic intextareaPropsis a nice touch.packages/design-system/index.ts (1)
47-49: Exporting the Markdown component via index.ts is consistent with other exports.This follows the existing pattern and improves discoverability by allowing consumers to import the
Markdowncomponent and its types directly from the design-system index. No further issues identified.packages/components/modules/messages/MessagesList/MessagesGroup/UserMessage/MessageItem/index.tsx (2)
4-4: Importing the Markdown component is a good step towards rendering rich text.This addition cleanly reuses the design-system’s Markdown component. Just verify that there aren’t any unused imports left in the file, and ensure that you are bundling only relevant icons.
40-41: Replacing raw text with the Markdown component is an excellent move.You have commented out the raw text usage in
Typographyand introduced<Markdown>. This is a clean approach, preserving formatting while also supporting additional features such as links and bold/italic text.packages/design-system/components/images/LazyLoadImage/index.tsx (2)
54-72: Ensure placeholders exist and are optimized.You’re correctly handling placeholders by providing a transparent image or a blur effect. If you expect large images, consider using a lightweight placeholder (e.g., a small low-resolution or base64-encoded image) to optimize page performance.
86-115: Excellent ratio-based styling approach.Using the
getRatiohelper for aspect ratios is clean. Keep in mind that if you handle videos or unusual image sizes, you may need to expand logic to avoid content shifting.packages/components/modules/messages/SendMessage/index.tsx (1)
128-130: Introducing the MarkdownEditor is a nice enhancement.Passing
MarkdownEditortoSocialTextFieldPropsis straightforward and cleanly encapsulated. Ensure you thoroughly test the user experience, especially around editor initialization times or in lower-bandwidth scenarios.packages/design-system/components/Markdown/styled.ts (2)
3-4: Consider clarifying the fallback for dark mode.
You’re assigninglightModebased ontheme.palette.mode === 'light', but there’s no direct check fortheme.palette.mode === 'dark'. If another mode is introduced or if the theme mode is set differently in some contexts, the style logic may produce unexpected behavior.
72-91: Ensure consistent code highlighting background in dark mode.
backgroundColor: lightMode ? theme.palette.grey[900] : alpha(theme.palette.grey[500], 0.16)yields a dark background even when not in dark mode. Verify that this color difference is intentional for the code blocks in dark themes.packages/components/modules/messages/MessageUpdate/index.tsx (1)
149-151: Confirm fallback behavior for non-Markdown inputs.
SettingTextFieldtoMarkdownEditorwithinSocialTextFieldPropsis a nice integration. However, verify if other areas of the app that rely on plain text editing still work properly, or if they also default to Markdown.packages/design-system/package.json (1)
28-40: Validate newly added Markdown dependencies.
You’ve added@uiw/react-md-editor,highlight.js,react-markdown, and related plugins. Confirm that your bundling process, minification, and tree-shaking properly handle these new libraries to avoid bloating the final bundle.packages/design-system/components/inputs/SocialTextField/types.ts (3)
1-1: Glad to seeFCbeing used for typed functional components.This import from React is standard for ensuring type safety on custom functional components.
5-5: Import ofMarkdownEditorPropslooks correct.The referencing of
MarkdownEditorPropsis necessary for supporting Markdown editor components.
7-7:TextareaFieldPropsimport is correctly placed.No issues with this import; it aligns well with the existing structure.
| @@ -0,0 +1,47 @@ | |||
| import Link from '@mui/material/Link' | |||
| import ReactMarkdown from 'react-markdown' | |||
There was a problem hiding this comment.
Resolve pipeline failure for ESM imports vs. CommonJS.
A pipeline error indicates that CommonJS imports produce require calls, conflicting with ESM modules. Ensure your build config or bundler settings handle ESM imports properly. You may need to adjust your tsconfig.json or Babel configuration to enable ESM for react-markdown or vice versa.
🧰 Tools
🪛 GitHub Actions: Main Workflow
[error] 2-2: CommonJS module imports will produce 'require' calls but referenced file is an ECMAScript module and cannot be imported with 'require'
| // eslint-disable-next-line no-param-reassign | ||
| delete props.target | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using the delete operator on function parameters.
Deleting properties from an object can degrade performance and trigger deoptimizations. Instead, consider assigning undefined to remove target.
Here's how you could refactor:
- delete props.target
+ props.target = undefined📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line no-param-reassign | |
| delete props.target | |
| } | |
| // eslint-disable-next-line no-param-reassign | |
| props.target = undefined | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 20-20: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| isReply, | ||
| replyTargetName, | ||
| onCancelReply, | ||
| TextField = TextareaField, |
There was a problem hiding this comment.
Fix type mismatch introduced by the TextField prop.
The pipeline reports that the provided props type for TextField is not assignable. Ensure that the union of TextareaFieldProps and MarkdownEditorProps is handled properly in SocialTextFieldProps, or provide a type guard. For instance:
- TextField?: FC<TextareaFieldProps> | FC<MarkdownEditorProps>
+ TextField?: FC<TextareaFieldProps & Partial<MarkdownEditorProps>>or maintain a strict union with type narrowing inside SocialTextField:
if (isMarkdownEditor(TextField)) {
// handle MarkdownEditorProps
} else {
// handle TextareaFieldProps
}Adjusting these definitions and usage will resolve the TS error and pipeline failure.
Also applies to: 51-51
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
678baec to
1f141a4
Compare
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
2 times, most recently
from
e801459 to
a21158f
Compare
|
@anicioalexandre and @Ronan-Fernandes does this PR still makes sense? If no, can we close it? If yes, can you please rebase @Ronan-Fernandes ? |
Fixes after rebase
Ronan-Fernandes
force-pushed
the
BA-2167-FE-markdown-editor
branch
from
aed2b4f to
88d0f9d
Compare
|
anicioalexandre
added
needs changes
__package_name__package update -v __package_version__Summary by CodeRabbit
New Features
Improvements
Design System Updates