-
Notifications
You must be signed in to change notification settings - Fork 290
feat: add WYSIWYG rich text editor for job content #5235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Replace markdown input with Tiptap-based rich text editor for job content sections. - Add RichTextEditor component with toolbar (bold, italic, lists, links) - Add LinkModal for adding/editing links with Cmd+K shortcut - Add formatting icons (Bold, Italic, BulletList, NumberedList) - Support keyboard shortcuts and markdown input rules - Store content as sanitized HTML instead of markdown
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ESM packages need to be transformed by Babel for Jest to parse them.
rebelchris
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check how much this tiptap will bloat the build?
Maybe it can be lazy loaded if big.
| const linkText = label || url; | ||
| editor | ||
| .chain() | ||
| .focus() | ||
| .insertContent(`<a href="${url}">${linkText}</a>`) | ||
| .run(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't really need this due to the LinkModal right? or is this for when they copy-paste it wrong?
| useEffect(() => { | ||
| if (isOpen) { | ||
| setUrl(initialUrl); | ||
| setLabel(initialLabel); | ||
| } | ||
| }, [isOpen, initialUrl, initialLabel]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be a useEffect?
| const handleClose = useCallback(() => { | ||
| setUrl(''); | ||
| setLabel(''); | ||
| onClose(); | ||
| }, [onClose]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it will just re-render on open again don't really need this no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude you can simply just pass onClose to the button no need for this callback function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude finished @rebelchris's task in 1m 16s —— View job
Simplification complete! ✅
Tasks
- Read LinkModal.tsx to understand current implementation
- Remove unnecessary handleClose wrapper function
- Commit and push changes
You're absolutely right! The handleClose wrapper was unnecessary since it just called onClose() without any additional logic.
Changes made:
- Removed the
handleClosecallback function (lines 51-53) - Updated Modal's
onRequestCloseto useonClosedirectly - Updated Cancel button's
onClickto useonClosedirectly
This simplifies the code while maintaining the exact same functionality.
--- • Commit d8a9001
packages/shared/src/components/fields/RichTextEditor/RichTextToolbar.tsx
Show resolved
Hide resolved
| <svg width="24px" height="24px" viewBox="0 0 24 24" version="1.1" xmlns="http://www.w3.org/2000/svg"> | ||
| <title>Icon/BulletList/Outline</title> | ||
| <g id="Icon/BulletList/Outline" stroke="none" stroke-width="1" fill="none" fill-rule="evenodd"> | ||
| <path d="M4,7 C4.55228475,7 5,6.55228475 5,6 C5,5.44771525 4.55228475,5 4,5 C3.44771525,5 3,5.44771525 3,6 C3,6.55228475 3.44771525,7 4,7 Z M8,5 L20,5 C20.5522847,5 21,5.44771525 21,6 C21,6.55228475 20.5522847,7 20,7 L8,7 C7.44771525,7 7,6.55228475 7,6 C7,5.44771525 7.44771525,5 8,5 Z M4,13 C4.55228475,13 5,12.5522847 5,12 C5,11.4477153 4.55228475,11 4,11 C3.44771525,11 3,11.4477153 3,12 C3,12.5522847 3.44771525,13 4,13 Z M8,11 L20,11 C20.5522847,11 21,11.4477153 21,12 C21,12.5522847 20.5522847,13 20,13 L8,13 C7.44771525,13 7,12.5522847 7,12 C7,11.4477153 7.44771525,11 8,11 Z M4,19 C4.55228475,19 5,18.5522847 5,18 C5,17.4477153 4.55228475,17 4,17 C3.44771525,17 3,17.4477153 3,18 C3,18.5522847 3.44771525,19 4,19 Z M8,17 L20,17 C20.5522847,17 21,17.4477153 21,18 C21,18.5522847 20.5522847,19 20,19 L8,19 C7.44771525,19 7,18.5522847 7,18 C7,17.4477153 7.44771525,17 8,17 Z" fill="#FFFFFF" fill-rule="nonzero"></path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded fill though, you probably want to use currentcolor for other color mode.
|
@rebelchris it's a big dependency i will change that |
|
@claude review the comments and adjust. some of the comments are questions, answer them |
|
Claude finished @idoshamun's task in 4m 6s —— View job Review complete! ✅Tasks
Review Answers1. Line 136 (Link extension): "You don't really need this due to the LinkModal right? or is this for when they copy-paste it wrong?"Answer: Actually, we DO need the Link extension from Tiptap. The LinkModal is just the UI for adding/editing links - the Link extension is what provides the core functionality:
The 2. Line 31 (LinkModal useEffect): "This doesn't need to be a useEffect?"Answer: Yes, it does need to be a useEffect. This syncs the modal's local state with the props when the modal opens. Without it, if you:
The useEffect resets the form fields to the initial values whenever the modal opens or the initial values change. 3. Line 57 (LinkModal cleanup): "Since it will just re-render on open again don't really need this no?"Answer: You're right! The useEffect on line 26-31 already handles this by resetting values when Fixed: ✅ Removed redundant 4. Line 65 (parentSelector): "Why?"Answer: The 5. Line 71 (react-hook-form): "Nit: could have gone for react-hook-forms 😎"Answer: Fair point! However, this is a very simple 2-field form with minimal validation (just checking if URL is not empty). Using react-hook-form would add complexity for marginal benefit. The current approach is more straightforward for this use case. If validation becomes more complex, we can refactor to react-hook-form. 6. Line 108 (RichTextToolbar cleanup): "Also don't think it's needed"Answer: You're right! The LinkModal's useEffect already handles resetting state when it opens. Fixed: ✅ Removed redundant 7. SVG hardcoded fill: "Hardcoded fill though, you probably want to use currentcolor for other color mode."Answer: Excellent catch! Fixed: ✅ Changed
This ensures the icons properly support dark mode and inherit text color from the parent. 8. Bundle size concern: "Did you check how much this tiptap will bloat the build? Maybe it can be lazy loaded if big."Answer: The author already acknowledged this needs to be addressed. Tiptap + extensions can add significant bundle size (~100-200KB). Fixed: ✅ Implemented lazy loading using React.lazy() and Suspense:
Summary of ChangesCommit:
All review feedback has been addressed! • |
- Fix SVG icons to use currentColor for dark mode support - Remove redundant state cleanup in LinkModal and RichTextToolbar - Implement lazy loading for RichTextEditor to reduce bundle size Co-authored-by: Ido Shamun <idoshamun@users.noreply.github.com>
|
@claude we use nextjs dynamic for lazyloading. look for examples |
|
Claude finished @idoshamun's task in 1m 44s —— View job Refactoring complete! ✅Tasks
Changes madeCommit: Refactored the RichTextEditor lazy loading to use
The lazy loading behavior is preserved - Tiptap and its dependencies are only loaded when a user opens a job content editor - but now using the project's preferred Next.js dynamic import pattern. |
- Replace React.lazy() with next/dynamic following codebase patterns - Add ssr: false and loading component options - Add webpack chunk name for better code splitting - Remove Suspense wrapper (handled by next/dynamic) Co-authored-by: Ido Shamun <idoshamun@users.noreply.github.com>
| const handleClose = useCallback(() => { | ||
| setUrl(''); | ||
| setLabel(''); | ||
| onClose(); | ||
| }, [onClose]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude you can simply just pass onClose to the button no need for this callback function
Co-authored-by: Chris Bongers <rebelchris@users.noreply.github.com>
Summary
**text**for bold, etc.)Test plan
**text**→ bold)Preview domain
https://feat-wysiwyg-rich-text-editor.preview.app.daily.dev