Skip to content
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

Merged
merged 1 commit into from
Jun 15, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 15, 2017

closes #1082

This PR adds a placeholder to empty posts. It takes the approach proposed by @jasmussen here #1082 (comment)

screen shot 2017-06-15 at 14 50 35

There's no default block selector for now. I guess this could be a global setting.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label Jun 15, 2017
@youknowriad youknowriad self-assigned this Jun 15, 2017
@jasmussen
Copy link
Contributor

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 }>
Copy link
Member

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?

Copy link
Contributor Author

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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@aduth aduth left a 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 👍

@jasmussen
Copy link
Contributor

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.

@youknowriad
Copy link
Contributor Author

you have to click twice just to start typing again below it.

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?

@jasmussen
Copy link
Contributor

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.

@youknowriad
Copy link
Contributor Author

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.

@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.

@youknowriad youknowriad merged commit fc8b865 into master Jun 15, 2017
@jasmussen
Copy link
Contributor

Are there accessibility concerns in setting focus inside the text box once clicked?

@youknowriad youknowriad deleted the try/empty-post-placeholder branch June 15, 2017 14:13
@aduth
Copy link
Member

aduth commented Jun 15, 2017

In testing this again on master, I'm not noticing any hover effects. Did this regress somehow?

@youknowriad
Copy link
Contributor Author

youknowriad commented Jun 15, 2017

@aduth you're right, I found that it's due to the switch to input because the input has no :before content.

I'll see if I can tweak the styling without using :before

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty post placeholder
3 participants