-
Notifications
You must be signed in to change notification settings - Fork 3
feat: incorporating the tiptap in articles CRUD operations #2693
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
for more information, see https://pre-commit.ci
merging with main branch
174efdc to
7783bac
Compare
1313fb6 to
193468e
Compare
|
|
||
| .simple-editor-wrapper { | ||
| width: 100vw; | ||
| height: 100vh; |
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.
Let's override these from our parent code to avoid modifying ejected library css. This should smooth the ride when we upgrade to new Tiptap versions.
You can do e.g.
styled(TiptapEditor)({
".simple-editor-wrapper": {
width: "auto",
height: "auto"
}
})
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.
ok done
193468e to
fd4b9ff
Compare
fd4b9ff to
d602fcb
Compare
jonkafton
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.
I left some comments. Let's improve the layout by pinning the toolbar to the header as we discussed (assuming not too much effort).
| className="input-field" | ||
| /> | ||
|
|
||
| <ClientContainer className="editor-box"> |
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.
From a previous PR, but this classname isn't referenced - can be removed
|
|
||
| return ( | ||
| <RestrictedRoute requires={Permission.ArticleEditor}> | ||
| <Container className="article-wrapper"> |
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.
From a previous PR, but this classname isn't referenced - can be removed
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.
done
| margin: "10px 0", | ||
| }) | ||
|
|
||
| styled(TiptapEditor)({ |
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 won't do anything unless used, ie.
const StyledTiptapEditor = styled(TiptapEditor)({ ... })
... and then use StyledTiptapEditor in the component
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.
we don't need this for now lets remove it
| const [text, setText] = useState("") | ||
| const [json, setJson] = useState({}) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const [json, setJson] = useState<Record<string, any>>({ |
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.
@tiptap/core exports type JSONContent so we can do useState<JSONContent>. We don't want to install @tiptap/core in the main workspace, but could re-export from ol-components as TiptapJSONContent, or better - add an export map to the ol-components package.json so we can do
import type { JSONContent } from "ol-components/tiptap-editor"
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.
done
| const [text, setText] = useState("") | ||
| const [json, setJson] = useState({}) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const [json, setJson] = useState<Record<string, any>>({ |
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.
JSONContent type here as well.
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.
done
| jest.mock("ol-components", () => { | ||
| // Reuse other exports from ol-components if needed | ||
| const actual = jest.requireActual("ol-components") | ||
| return { | ||
| ...actual, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| TiptapEditor: ({ value, onChange, "data-testid": testId }: any) => { | ||
| return ( | ||
| <textarea | ||
| data-testid={testId || "editor"} | ||
| value={JSON.stringify(value, null, 2)} | ||
| onChange={(e) => { | ||
| try { | ||
| // Allow simulating JSON-like updates if needed | ||
| const parsed = JSON.parse(e.target.value) | ||
| onChange?.(parsed) | ||
| } catch { | ||
| // fallback to raw string for simple tests | ||
| onChange?.(e.target.value) | ||
| } | ||
| }} | ||
| /> | ||
| ) | ||
| }, | ||
| } | ||
| }) | ||
|
|
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.
We shouldn't need to mock the editor here. For the actual instance to work, we'll need two things:
- the data-testid to be passed to TiptapEditor and set on its containing div for the
expect(screen.getByTestId("editor")).toBeInTheDocument()assertion.SimpleEditorPropscould extendReact.HTMLAttributes<HTMLDivElement>so we can set any div props on the contiainer, or just:
interface SimpleEditorProps {
value?: object
onChange?: (json: object) => void
readOnly?: boolean
"data-testid"?: string
}
- the article factory must return a valid document on the content, e.g.
const article = factories.articles.article({
id: 42,
title: "Existing Title",
content: {
type: "doc",
content: [
{
type: "paragraph",
content: [{ type: "text", text: "Existing content" }],
},
],
},
})
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.
done
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.
If we need a placeholder image, let's use one already in the repo, e.g. https://github.com/mitodl/mit-learn/blob/main/frontends/main/public/mit-dome-2.jpg
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.
done
848f12f to
bb13dcd
Compare
bb13dcd to
a42a4c5
Compare
jonkafton
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.
👍 #2699 to follow up
What are the relevant tickets?
In reference to #2691
Description (What does it do?)
This is about to consume the tiptap component that is created in #2691 and make changes in unit test as well
Screenshots (if appropriate):
Screen.Recording.2025-11-11.at.5.54.13.PM.mov
How can this be tested?
Additional Context