Skip to content

Pullquote: Adding block alignments and using proper placeholder#1268

Merged
youknowriad merged 6 commits intomasterfrom
update/pullquote-alignments
Jun 21, 2017
Merged

Pullquote: Adding block alignments and using proper placeholder#1268
youknowriad merged 6 commits intomasterfrom
update/pullquote-alignments

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

closes #1221
closes #1222

This PR enhances the pullquote block by adding block level alignment controls and is using a proper placeholder instead of default values.

@youknowriad youknowriad added the [Feature] Blocks Overall functionality of blocks label Jun 19, 2017
@youknowriad youknowriad self-assigned this Jun 19, 2017
@youknowriad youknowriad requested a review from jasmussen June 19, 2017 15:54
@jasmussen
Copy link
Copy Markdown
Contributor

Nice! Thanks for working on this.

When left floated, there's an issue with the layout. I think we need to port the following from being image only, to being for all things left aligned:

max-width: 370px;
width: 100%;

If you remove the placeholder text in the main body of the quote, you get an empty pullquote entirely:

screen shot 2017-06-20 at 10 22 55

This is an interesting state. If you empty out the main field we should at least have "Write quote..." in the main. But the edgecase here is: do we also want it to show when the block is unselected?

@youknowriad
Copy link
Copy Markdown
Contributor Author

@jasmussen I've fixed the placeholder behaviour and I could use some help with the float left styling issue. I'm not sure I understand correctly and I'm not really aware of how these positioning work right now.

@jasmussen
Copy link
Copy Markdown
Contributor

Nice!

I pushed a little alignment fix. More work needs doing, but I'll ticket/do that separately. I think we should get this in.

@youknowriad youknowriad requested review from aduth and nylen June 20, 2017 18:58
! content ||
! content.length ||
// On non-inline editables, TinyMCE appends an empty <p> tag
( ! this.props.inline && content.length === 1 && ! content[ 0 ].props.children ),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we hook into TinyMCE's internal empty checking?

https://github.com/tinymce/tinymce/blob/master/src/core/src/main/js/dom/Empty.js

I was wondering why we wouldn't have had to account for br[data-mce-bogus] which is added similar to empty <p> tag here. Appears we do some clean-up in getContent already. Should we be stripping the empty paragraphs there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thihs works great 👍

value: nextValue,
} )
}
placeholder={ ( 'Write Quote…' ) }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot __ 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hahaha, hard to notice because it works

@youknowriad youknowriad force-pushed the update/pullquote-alignments branch from 0e78102 to d2d4a83 Compare June 21, 2017 10:55
@youknowriad youknowriad force-pushed the update/pullquote-alignments branch from d2d4a83 to 9efa4c4 Compare June 21, 2017 14:19
@youknowriad youknowriad merged commit 3c9f47a into master Jun 21, 2017
@youknowriad youknowriad deleted the update/pullquote-alignments branch June 21, 2017 14:29
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.

Pullquote: Needs to use proper placeholder text component Pullquote: Needs block level alignments

3 participants