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

Plugin: Add Heading Block #337

Closed
wants to merge 2 commits into from

Conversation

BE-Webdesign
Copy link
Contributor

Accidentally made a mess of git here is a clean version adding in JSX and making use of t he Editable component.

@BE-Webdesign BE-Webdesign changed the title Add heading block with JSX and Editable component Plugin: Add Heading Block Mar 28, 2017
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Aside the default as h2, this LGTM 👍 for a v1 of the heading block.

Now, we have some work to do on the Editable side to ensure we can not create paragraphs etc... (but this is transversal to different blocks, same for text for example)

},

edit( attributes, onChange ) {
const { headingType = 'H1', value } = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use h2 by default to keep h1 for the post title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.

@@ -1 +1,2 @@
import './text-block';
import './heading-block';
Copy link
Member

Choose a reason for hiding this comment

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

We could drop the -block suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do.


save( attributes ) {
const { headingType = 'H1', value } = attributes;
return <Heading nodeName={ headingType }>{ value }</Heading>;
Copy link
Member

@mtias mtias Mar 28, 2017

Choose a reason for hiding this comment

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

Why not type={ headingType } as the prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I went back and forth on that. Technically the value for the property is the nodeName and not the tag value. So I left it as nodeName. I basically just copied what aduth wrote because it looked good lol.

@mtias mtias added [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Mar 28, 2017
@BE-Webdesign
Copy link
Contributor Author

BE-Webdesign commented Mar 28, 2017

We will need some way to change the forced_root_block value for TinyMCE on heading blocks. Otherwise, inside the Heading, there will always be a <p> tag wrapping around the content, which throws off the typography.

@ellatrix
Copy link
Member

I think we should try to have <h1 contenteditable="true">{ ...children }</h1> instead of messing with forced_root_block?

@BE-Webdesign
Copy link
Contributor Author

@iseulde Sounds like a potentially better solution. From my understanding, that would mean not using TinyMCE for the heading block? What will we lose out on if we use

<h1 contenteditable="true">{ ...children }</h1>

instead?

@ephox-mogran
Copy link
Contributor

Hey,

Have you considered making tinymce use the heading itself as the target container, rather than embedding a div inside the heading? I see at the moment that the Editable react component always creates a div and uses that as the rendered element, but you could make its node name configurable. Then you wouldn't have to worry about forced_root_block (this is probably what Ella is suggesting).

If you want an example of using a heading tag for an inline tinymce instance, see the examples here: https://www.tinymce.com/docs/demo/inline/

@youknowriad
Copy link
Contributor

@ephox-mogran What are the pros of this approach compared to always using a div container?

@ephox-mogran
Copy link
Contributor

It gives tinymce more context for the various operations. With differently structured blocks (like ol and ul), it prevents the insertion of a div tag from causing problems with the HTML structure. Primarily, it means that tinymce is given more information to make decisions, and more of the finer formatting details can be delegated to tiny.

When a tinymce instance is created on a target node, it's able to use that node to govern how it should act (with regards to Enter key behaviour etc). If you are embedding tinymce inside something and then trying to give tinymce restrictions based on information that you know but it doesn't, it's probably going to get a lot more complicated.

@youknowriad
Copy link
Contributor

@ephox-mogran Thanks for the explanation, Good points here. I guess the more we build blocks (heading, quotes, lists, text ...), the more we'll have context on whether we should do this or not. (Especially when we'll introduce keyboard interactions and HTML restrictions)

@aduth
Copy link
Member

aduth commented Mar 31, 2017

See #327 (comment) for relevant changes to edit and save function signature.

@ellatrix ellatrix mentioned this pull request Apr 14, 2017
@BE-Webdesign
Copy link
Contributor Author

Closing in favor of #423

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 Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants