Allow Editable to have variable node name.

[ellatrix]

No description provided.

[youknowriad]

🤔 This PR address two things at the same time:

Container included in content or not

1- We have different use cases to consider here, Most blocks need Editable component to handle the content of the blocks. I mean when using <Ediable value={ value } onChange={ callback } /> We pass a raw value without the container tag, so logically, we'd expect the callback to be called with a value where the container tag added automatically by TinyMCE has been stripped out (like you suggested in Slack).

2- We have some other blocks where we don't want to strip out the container added by TinyMCE, typically, multi-paragraphs blocks, blockquote content ... So, that's why I'll suggest an inline prop to say if want the container or not. Something like <Editable value onChange inline />

Customizing TinyMCE's node

I personally don't think we should add a nodeName prop to change the container because this should be internal implementation details of the Editable component regardless of how it renders its content, its role is to handle the formatting and the updating of this content. So using an extra div as an internal implementation detail doesn't bother me at all.

[joyously]

Will the extra div or lack of it affect how the theme has to write the editor styles? Like sometimes it has the container and sometimes not? Or should it be consistent?

[youknowriad]

@joyously The extra div is not saved to the post content, it's just here while editing. It does not affect the theme styling. It can affect how "we" (block authors) style the block editing interface.

Edit: on the per-block prototype, the div didn't affect the editor styling at all

[joyously]

I'm referring to the editor-style.css that the theme provides so that it looks like it will on the front end. Unless we're doing that a different way with this editor.

[youknowriad]

@joyously That's a good question! I don't think this is settled yet. It'll probably work in a different way, I don't expect the theme authors to be able to inject any style affecting the Editor UI (blocks, controls, toolbars etc...). I think the iframe distinction we have in the current TinyMCE editor allows us to inject any styling in the post content.

Others may have better ideas here @mtias @nylen

[youknowriad]

After some 🤔 , I'm finding this valuable for other blocks (could ease writing some blocks like list #358). But it transforms the current text block to a single p block which could be changed later.

Let's get this merged 👍

Though, I prefer nodeName over name for the prop name

[ellatrix]

Don't have a preference for naming 👍

…

On 31 Mar 2017, at 10:37, Riad Benguella ***@***.***> wrote: @youknowriad approved this pull request. After some 🤔 , I'm finding this valuable for other blocks (could ease writing some blocks like list #358). But it transforms the current text block to a single p block which could be changed later. Let's get this merged 👍 Though, I prefer nodeName over name for the prop name — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[aduth]

Just to add to naming confusion, I'd personally prefer tagName. In the sense of an HTMLElement, the nodeName property value is always capitalized. tagName is the terminology used for createElement in earlier versions of the specification and in MDN documentation. Newer specifications ([1], [2]) define it as localName, which I'm not so much a fan of. name is a little too generic and I worry would be confused with the DOM attribute of the same name.

[ellatrix]

tagName works too for me. Let's do that.

[ellatrix]

Since only one paragraph is expected, I used attributes.value[0]. Not sure if this is best.

[aduth]

Should we define a default tagName if none is specified, or should we assume that this must be specified by the block implementation?

[aduth]

If you're only using the first paragraph, this can be simplified to:

value: html( 'p' )

query will return an array of values scoped to each matching element for the selector, useful if we'd want to consider multiple paragraphs in this block.

[aduth]

Rebased and incorporated feedback from my latest comments.

I also got a little carried away with how best to allow the <Editable /> component to define its own styles without setting a cumbersome precedent of assets/stylesheets in each top-level directory. Instead, now each component imports its styles directly.

I've lately been coming around to the idea that perhaps the blocks and editor directories should be merged together, but they each serve independent purposes (at least as far as we'll expect plugins to interact with them). And to a more minor point the folder setup makes it easier in Webpack to expose globals. We can discuss this separately, but I figured I'd raise it since it was a pain point with the changes.

[aduth]

It seems important to keep these line numbers up to date, but I worry the changes will be noisy in pull requests. There's also a chance that the developer doesn't run npm run build or npm run dev after making changes.

It could be neat to set up a bot which automatically commits any necessary .pot updates directly to master after a merge, though it's uncertain how this could play out long-term with core integration.