-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(placeholder): hide placeholder in atom nodes by default #6044
base: develop
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 9064e80 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Could you please rebase this branch from |
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.
Looking good, thanks for addressing this!
@@ -103,8 +113,9 @@ export const Placeholder = Extension.create<PlaceholderOptions>({ | |||
doc.descendants((node, pos) => { | |||
const hasAnchor = anchor >= pos && anchor <= pos + node.nodeSize | |||
const isEmpty = !node.isLeaf && isNodeEmpty(node) | |||
const isContentEditable = this.options.showWhenAtom ? !node.type.isAtom : true; |
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 var name doesn't really describe it 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.
So the atom is not considered a leaf? That is surprising to me? And it also isn't empty? That sounds wrong to me
Either the isNodeEmpty should be updated to make it be counted as being empty or something else is up here
* If false, the placeholder will not be shown for atom nodes. | ||
* @default false | ||
*/ | ||
showWhenAtom: boolean; |
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 disagree with making this an option, people should not have to care. You can also just disable the placeholder with css so I don't see why we would need to even modify the JS here
Changes Overview
This update enhances the Tiptap placeholder extension by preventing placeholders from being displayed on atom nodes by default. This ensures a more intuitive user experience by only showing placeholders in meaningful contexts.
Motivation
Atom nodes are typically self-contained and do not require placeholders. Displaying placeholders on them can create visual clutter and confusion. By default, this change hides placeholders on atom nodes, improving clarity. However, for flexibility, a new option
showWhenAtom: true
allows placeholders to be shown on atom nodes when needed.Implementation Approach
The extension now checks if a node is an atom before rendering the placeholder. If showWhenAtom is set to true, placeholders will still be displayed on atom nodes.
Visual Changes
Before
After
Checklist