-
Notifications
You must be signed in to change notification settings - Fork 4.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
Blocks: Adding a placeholder when the post has no blocks #1195
Conversation
Holy guacamole I can't believe how fast you are moving! Go go go! |
@@ -164,7 +172,12 @@ class VisualEditorBlockList extends wp.element.Component { | |||
|
|||
return ( | |||
<div> | |||
{ blocksWithInsertionPoint.map( ( uid ) => { | |||
{ ! blocks.length && ( | |||
<button className="editor-visual-editor__placeholder" onClick={ this.appendDefaultBlock }> |
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.
Should this be onFocus
so it becomes activated on tabbing from the title as 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.
I guess we need both because Firefox and Safari don't focus on click in MacOS. Or better transform this to an input
@@ -255,3 +255,39 @@ $sticky-bottom-offset: 20px; | |||
content: ''; | |||
} | |||
} | |||
|
|||
.editor-visual-editor__placeholder { |
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.
Are there common styles here we could extend from a base [class/SASS mixin/SASS placeholder] (or combine into a single selector) between the placeholder and actual blocks?
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.
Yes, I guess the "centering" behaviour could be extracted.
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.
It's hard to extract something meaningful really. I don't like extracting shared styles just because they are the same. My questions here are:
- How should we name the extracted shared CSS mixin?
- Is it really worth it if it's shared between two elements only
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 don't like extracting shared styles just because they are the same.
Okay, I'd not recommend it if they're not truly "shared". I thought we were emulating block effects by duplicating them here. We can leave it as-is.
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 thought we were emulating block effects by duplicating them here.
Yes, this is what we're doing but hard to extract something meaning full without overriding a lot of styles.
857a8a0
to
f6a504c
Compare
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.
Tabbing from the title to the placeholder block does not cause focus styles to be removed from the title. Not sure if that's specifically related to the changes here or not.
Otherwise, looks good from code perspective 👍
Nice, looks good! Looks like it has potential. I can't review as I'm in an airport, but it seems like it can help solve the issue where if you insert an image or other non text block, you have to click twice just to start typing again below it. @mtias had some concerns that it might be simpler to start with an actual text field, so might want his eyes, but from what I can see here, it seems like a great start. |
For now I'm only showing this if the post is empty, are you suggesting we always show this placeholder at the end of the post? |
That was the initial idea but that idea is not yet fully formed, I think that needs a mock-up. So it's probably good to start simple. |
@aduth Yes, not related to this issue. I tried to come up with a quick fix here but it looks like it's harder than expected because we need to keep the focus styles when we click on the Copy permalink button. |
Are there accessibility concerns in setting focus inside the text box once clicked? |
In testing this again on master, I'm not noticing any hover effects. Did this regress somehow? |
@aduth you're right, I found that it's due to the switch to I'll see if I can tweak the styling without using |
closes #1082
This PR adds a placeholder to empty posts. It takes the approach proposed by @jasmussen here #1082 (comment)
There's no default block selector for now. I guess this could be a global setting.