-
-
Notifications
You must be signed in to change notification settings - Fork 642
fix: remove dependency array from comments re-rendering #2177
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
|
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: |
ed58642 to
f6b6dd4
Compare
f6b6dd4 to
3731ac9
Compare
3731ac9 to
746b15a
Compare
YousefED
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 think you're right. Can you make sure with manual testing this doesn't cause stale data outside of the edge-case you mentioned?
|
I've just gone with the React.key, to make doubly sure. This should be exactly the same behavior now. |
Summary
I think that this was being used to allow the editor to re-render if it saw a new comment.body, but the only scenario that I can even imagine that happening is that the same person is logged in on two different browsers & updates their comment on one and has the comment editor open on the other. In that case they will now see a stale value. If we really cared about this case, then we can change the Comments component to use a React.key that forces the Comment component to re-render based on the new comment body content, but I think it is enough of an edge case that it isn't even worth implementing that.
Rationale
Changes
Impact
Testing
Screenshots/Video
Checklist
Additional Notes