Social Links: Add text placeholder next to appender#29851
Merged
Conversation
|
Size Change: +308 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
7 tasks
7227c41 to
1575345
Compare
Contributor
|
Don't think I didn't see what you did here! That said, thanks for the PR. I rebased and pushed some polish:
The only thing here is that the text isn't clickable. I could potentially hack that to work, but I'm not sure. So I'm going to think more about what to do here, and I'd welcome input as well. |
Contributor
|
@mkaz I'd love to land this one! Any thoughts on clickable text? Or should we rephrase to sound less clickable and point to the plus? |
Member
Author
|
@jasmussen So I think if we want the text to be clickable, we'll want to alter the The way the placeholder works is it gets passed forward to the block list and gets rendered as an element in at the same level of the appender, ie. it is not a part of the appender. You can see that code here Let me test out a way to alter the appender and add the text in front which I believe will work. Though I feel we went down this path a year or two ago, but can't find the reference or why we didn't go forward with it. 🤷♂️ |
Member
Author
|
I don't think it is as straight-forward as I hoped. A way to solve would be to create a I think a better solution is to create a label or prop that can be passed down to the Inserter and placed alongside the button with both being the same click action. This would allow other InnerBlocks consumers, such as Navigation, to use and not have to also create their own I created ticket #30688 that can address the larger issue, for this one maybe we just alter the text for now and 🚢 Looking back at the original ticket for placeholder here #25941 I had stubbed out the text |
793fb20 to
987c7cc
Compare
Contributor
|
Okay, rebased and tweaked. Let's try this:
|
Member
Author
|
Approved 👍 (It is my own PR, so that can't be the official one) |
jasmussen
approved these changes
Apr 13, 2021
jasmussen
left a comment
Contributor
There was a problem hiding this comment.
Let's try it. Current state in trunk is not great.
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Create a SelectedPlaceholder to show text next to the appender when selected.
This helps the guide the initial empty state and adding the first links.
Related to #29756
How has this been tested?
Screenshots
Types of changes
Adds new SelectedPlaceholder components and switches what gets passed to InnerBlocks based on selected state