Improve editor binding presence support#985
Conversation
There was a problem hiding this comment.
Pull request overview
Improves the Storybook side-by-side demos for the collaborative Slate.js and ProseMirror integrations by adding richer introductory content and enhancing code styling in the ProseMirror editor demo.
Changes:
- Added Markdown-based descriptions and updated initial demo documents to better explain each integration.
- Updated Slate/ProseMirror demo content to include installation and binding examples.
- Customized ProseMirror schema
toDOMforcode/code_blockand tweaked editor container padding.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/collaborative-slate/src/stories/side-by-side.stories.tsx | Adds a richer description + updates the initial Slate document used to seed the CRDT. |
| packages/collaborative-prosemirror/src/stories/side-by-side.stories.tsx | Adds a richer description + updates the initial ProseMirror JSON used to seed the CRDT. |
| packages/collaborative-prosemirror/src/stories/ProseMirrorEditor.tsx | Customizes code / code_block DOM rendering in the schema and adjusts editor styling. |
Comments suppressed due to low confidence (2)
packages/collaborative-prosemirror/src/stories/side-by-side.stories.tsx:80
- This story builds the initial ProseMirror JSON using a
mySchemadefined in this file, butProseMirrorEditornow uses a different schema (customNodes/customMarkswith overriddentoDOM). Keeping two schemas in sync is error-prone and can cause subtle inconsistencies. Consider exporting a shared schema (or a schema factory) fromProseMirrorEditorand reusing it here fornodeFromJSON()/FromPm.convert().
const model = ModelWithExt.create(ext.peritext.new(''));
const viewRange = FromPm.convert(mySchema.nodeFromJSON(json));
const txt = model.s.toExt().txt;
packages/collaborative-slate/src/stories/side-by-side.stories.tsx:69
- In the Slate demo document content, the code snippet uses
bind(peritextRef, editor)butperitextRefis not shown/defined anywhere in the snippet. Since the DESCRIPTION above uses the correct pattern() => model.s.toExt(), consider aligning the snippet to showconst peritextRef = () => model.s.toExt();(or use the inline lambda) so readers can copy/paste it without guessing.
{
type: 'code-block',
children: [{text: "import {bind} from '@jsonjoy.com/collaborative-slate';\nconst unbind = bind(peritextRef, editor);"}],
},
| Install the package and its peer dependencies: | ||
|
|
||
| \`\`\`bash | ||
| npm install @jsonjoy.com/collaborative-slate slate slate-react | ||
| \`\`\` |
There was a problem hiding this comment.
The DESCRIPTION text says “Install the package and its peer dependencies”, but the command only installs slate/slate-react. This package also has peer deps on react, react-dom, and tslib (see packages/collaborative-slate/package.json). Consider either listing all peer deps in the install snippet, or rewording to avoid claiming the command installs all peer deps.
| Install the package and its peer dependencies: | ||
|
|
||
| \`\`\`bash | ||
| npm install @jsonjoy.com/collaborative-prosemirror prosemirror-model prosemirror-state prosemirror-view |
There was a problem hiding this comment.
The DESCRIPTION says “Install the package and its peer dependencies”, but @jsonjoy.com/collaborative-prosemirror declares only tslib as a peer dependency (the ProseMirror packages are regular dependencies and will be installed transitively). Suggest rewording this section and/or simplifying the install command to avoid inaccurate guidance.
| Install the package and its peer dependencies: | |
| \`\`\`bash | |
| npm install @jsonjoy.com/collaborative-prosemirror prosemirror-model prosemirror-state prosemirror-view | |
| Install the package: | |
| \`\`\`bash | |
| npm install @jsonjoy.com/collaborative-prosemirror |
| {type: 'text', text: ' cleanup function:'}, | ||
| ], | ||
| }, | ||
| {type: 'code_block', content: [{type: 'text', text: "const unbind = bind(peritextRef, facade);"}]}, |
There was a problem hiding this comment.
The ProseMirror demo content’s code block says const unbind = bind(peritextRef, facade);, but the API described above (and the actual code) uses PeritextBinding.bind(peritextRef, facade) (or another correctly imported symbol). As written, bind is undefined/misleading for users reading the demo content—please update the snippet to match the real binding API.
| {type: 'code_block', content: [{type: 'text', text: "const unbind = bind(peritextRef, facade);"}]}, | |
| {type: 'code_block', content: [{type: 'text', text: "const unbind = PeritextBinding.bind(peritextRef, facade);"}]}, |
No description provided.