-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Add renderPlaceholder #4190
Add renderPlaceholder #4190
Conversation
🦋 Changeset detectedLatest commit: 4344f53 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
- adjust types to enable passing renderPlaceholder prop to leaf component
thanks for the review @ianstormtaylor - I addressed your feedback 👍 |
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.
Hey @juliankrispel thanks for the update!
I was just going through to review again, and I think I realized that we're trying to solving a few different problems at once here with the same API, and I'm not sure we're doing it right. For a summary of how I'm thinking about this, we have a few problems we're solving for:
-
For the most common case, a simple
placeholder="..."
string is needed to mimic the default browser behavior that applies to<textarea>
elements. We want this to get the same styling that those placeholders do (namely the opacity). -
There's another common use case that is changing the rendering logic for when and where placeholders are rendered. This can be when implementing placeholders in the middle of documents (eg. in a caption block for an image), or when wanting to have multiple placeholders (eg. for a heading and body in a forced layout). I think these cases should be left unsolved by the
placeholder/renderPlaceholder
props, and instead people should use their own customdecorate
prop instead for maximum control. -
A less common but totally valid case is wanting to render more complex components in place of the default global placeholder. But the issue here is that people may or may not want to render them with the same styles as the default, specifically slate-react placeholder unable to change the style #4123 doesn't want the opacity styles. Right now we're forcing those styles, but we could tweak the logic to just use a
<DefaultPlaceholder>
similar to how we have a<DefaultLeaf>
, allowing them to override it withrenderPlaceholder
.
I added a few comments in line to explain what the changes would be to arrive at that. It's a little complex to understand, but the goal is to make renderPlaceholder
a very simple but fully customizable escape hatch that runs in the default code path.
Let me know if you have questions!
- separate renderPlaceholder method from placeholder prop - add custom-placeholder example
thanks for the tight feedback loop @ianstormtaylor much appreciated 🙏 I addressed your feedback but I believe there are still open questions Also added an example for custom placeholders. Lmk if there's anything else you'd like me to change. Cheers! |
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.
Thanks @juliankrispel, this is looking great. I realized you were totally right about mixing in the style
attribute, my bad for arguing against that. I put a few more comments in line for refining. I love the idea for the example of the customized placeholder!
- added attributes prop to default render prop - make renderPlaceHolder consistent with other props
Thank you for the review @ianstormtaylor, I much appreciate the help! I've addressed all the feedback in your most recent review, cheers |
…renderPlaceholder
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.
Everything looks great @juliankrispel! Two comments inline and then I'm good to merge.
thanks, all feedback addressed |
Description
Adds a
renderPlaceholder
prop toEditable
Issue
Fixes #4123
Fixes #4018 by removing
whitespace: nowrap
Example
Context
If your change is non-trivial, please include a description of how the new logic works, and why you decided to solve it the way you did. (This is incredibly helpful so that reviewers don't have to guess your intentions based on the code, and without it your pull request will likely not be reviewed as quickly.)
Checks
yarn test
.yarn lint
. (Fix errors withyarn fix
.)yarn start
.)yarn changeset add
.)