fix: BlockNoteViewEditor mismatched editable value
#2357
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
| ctx.editorProps.editable !== editor.isEditable | ||
| ) { | ||
| editor.isEditable = props.editable; | ||
| editor.isEditable = ctx.editorProps.editable; |
There was a problem hiding this comment.
I'm wondering if this is needed, don't we already do this on line 145? Can we find edge-cases why we also need to set this here on mount / why we added it originally?
There was a problem hiding this comment.
You're correct, it's redundant to have this logic in both components. Seems like the reason this was done is that the effect on line 145 would only fire after the context value was updated, so editor.isEditable may not have the correct value when mount is run.
I ended up removing the effect on line 145 and simplifying the code in mount instead, to not deal with this weird hook priority thing.
There was a problem hiding this comment.
Ah realized I commented on the same sort of behavior #2357 (comment) . I think we should be using only the effect & not the mount handler. If we need to have this render before layout (as some things will look different in editable vs. not mode), then use a useLayoutEffect since it will be done before paint. Though, I could see this triggering a re-render of the editor blocks that are defined as react nodes, so ideally it'd be set on editor creation & the layout effect is purely for reactivity
There was a problem hiding this comment.
Hmm, even with useLayoutEffect, mount is still called before the effect, so things still don't work. Also, I don't think we could set it on editor creation since editable can only be set on the view, not passed to the editor itself. Maybe worth discussing on call.
| editor.isEditable = props.editable; | ||
| } | ||
| // Set editable state of the actual editor. | ||
| editor.isEditable = ctx.editorProps.editable !== false; |
There was a problem hiding this comment.
Is it intentional that editable is not "reactive", i.e. you change that context value & it won't switch the editor between editable & not editable modes?
We should check this behavior in an example. Since I think people may have things like:
function Component(){
const [isEditable, setEditable] = useState(true);
return <><button onClick={()=>setEditable(a=>!a)}>Toggle</button><Editor editable={isEditable} /></>
}And, expect it to react to the button.
There was a problem hiding this comment.
Modified the existing example where this issue was coming up - all good, editor is re-mounted with the toggled editable state and everything works as expected. ctx.editorProps.editable is in the dependency array of mount, so I think this makes sense.
nperez0111
left a comment
nperez0111
left a comment
There was a problem hiding this comment.
Always appreciate something that simplifies things. Just want to check the one thing first
Summary
When using
BlockNoteViewEditorand settingrenderEditor={false}inBlockNoteViewComponent, you currently have to pass the sameeditableprop to both, or it causes issues.Because it doesn't make sense to need to pass the same
editableprop to 2 different components in the first place, this PR removes the prop fromBlockNoteViewEditor. Instead, the editable value is passed in through theBlockNoteContextto ensure it's the same between the components.Rationale
There is currently an issue with the comments sidebar example caused by this mismatch in editable states, as the prop is passed to
BlockNoteViewComponentbut notBlockNoteViewEditor.This makes the editor not focusable while the user selected has only comment priviledges and
editableisfalse. That, in turn, prevents the editor from being focusable, meaning the formatting toolbar can't be shown, and so the user can't add comments.Changes
See above
Impact
Testing
Technically a breaking change since
BlockNoteViewEditorno longer supports theeditableprop? Shouldn't really matter though.Screenshots/Video
N/A
Checklist
Additional Notes