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

Add/365 freeform block #880

Merged
merged 17 commits into from
Jun 5, 2017
Merged

Add/365 freeform block #880

merged 17 commits into from
Jun 5, 2017

Conversation

BoardJames
Copy link

The purpose of this pull request is to:

  • Make a basic freeform block

It follows this mockup:
#365 (comment)

@BoardJames
Copy link
Author

Note it currently looks like this:
basicfreeform

@jasmussen
Copy link
Contributor

From the ticket discussion, I think it'd be good to get the above very very basic version of the freeform block in, then we can iterate further in subsequent PRs.

@ellatrix
Copy link
Member

Should we avoid using Editable for this to keep it small? It also might start diverging if we choose a certain state structure or choose to make it purely inline.

@@ -0,0 +1,12 @@
.blocks-freeform .blocks-editable__tinymce {
ul, ol {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like spaces instead of tabs here.

Copy link
Author

Choose a reason for hiding this comment

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

Good catch. I have my editor in spaces mode by default - I've changed the workspace settings so this hopefully won't happen in future.

@BoardJames
Copy link
Author

@iseulde

Should we avoid using Editable for this to keep it small? It also might start diverging if we choose a certain state structure or choose to make it purely inline.

What do you propose instead? I have no idea how to improve the pull request based on that statement - though I get the impression you don't like it..?

@ellatrix
Copy link
Member

I'm just saying that we might start limiting the Editable component more in the future, in which case Editable doesn't seem like a suitable component for this? E.g. #898. The purpose of Editable is to keep it very small and it might not be the same as it is now. The purpose of the freeform block is to offer the same editing experience as WordPress core has now. Of course, we can revisit this.

What do you propose instead?

Maybe it's worth having a less limited TinyMCE component for this block specifically (in its folder).

Maybe @mtias has more thoughts on this.

@nylen
Copy link
Member

nylen commented May 26, 2017

After #849 which added a bunch of tests for block parsing and serialization, this PR should probably be rebased against master, either because the build will fail upon merge, or because this PR can add some new tests using this structure (details here).

@BoardJames
Copy link
Author

Thanks @nylen I'll look into it.

@BoardJames
Copy link
Author

@nylen
I had some internal state being serialized so I hid them using a helper function:

function internalValue( defaultValue ) {
	const thunk = () => defaultValue;
	thunk._wpBlocksKnownMatcher = true;
	return thunk;
}

registerBlock( 'core/freeform', {

	attributes: {
		content: children(),
		listType: internalValue( null ),
		inBlockQuote: internalValue( false ),
		editor: internalValue( null ),
	}, 
...

Is my method of hiding the internal state sensible or is there a better way?

@mtias
Copy link
Member

mtias commented May 29, 2017

@EphoxJames to expand on the role of Editable. The idea is that Editable remains as the smallest possible implementation of a basic TinyMCE for formatting text. Blocks that require other tinymce plugins, or which would benefit from more formatting options, should add tinymce on their own, or perhaps wrap Editable in a configurable manner. In this case, I believe we should add TinyMCE separate from Editable so that the purpose is clear.

@BoardJames
Copy link
Author

@mtias I have not changed Editable in this pull request - I am only using features already present in master. This to me seems that I am wrapping Editable in a configurable manner exactly as you request??? Do you mean some features on the master version are going to be removed? If so which ones?

I can't do anything if I continue to get very vague comments. I am getting quite frustrated at the lack of progress in this pull request and #850 but I don't know what else to do.

@nylen
Copy link
Member

nylen commented May 30, 2017

I am not sure if we have a way to handle internal state (if attributes are React component props, then we don't seem to have a corresponding mechanism for state). It would be good to get @aduth and/or @youknowriad to weigh in on this specific point and on this PR in general.

editor.on( 'nodeChange', ( nodeInfo ) => {
setAttributes( {
listType: findListType( nodeInfo ),
inBlockQuote: findInBlockQuote( nodeInfo ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Like @nylen mentioned, we shouldn't use setAttributes for "temporary" state that is not going to be persisted in the block output markup. We should use local state for this (the edit here can be a regular React Component).

You'll be able to access the local state in the controls if you embed the controls inside the edit function (see #830)

>
{ attributes.html }
</div>
<Editable
Copy link
Contributor

@youknowriad youknowriad May 30, 2017

Choose a reason for hiding this comment

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

Should we use Editable for this block? I don't know the answer here because we may think this block would be way more complex than the regular Editable usage. At the same time having access to the onSetup callback allows us to enhance the behaviour, so maybe we could try for now and reconsider later.

Copy link
Member

Choose a reason for hiding this comment

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

@BoardJames
Copy link
Author

@youknowriad I've created a react component and moved the state out of the attributes into the react component. Is there anything else needed?

@BoardJames
Copy link
Author

I've rewritten to replace Editable (since I hear it is becoming just contenteditable). I've also tried to hook into tinymce button state (still a work in progress). My intent is to use that knowledge to implement the format dropdown.

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.

This looks great as a V1 for me 👍
We're seeing some duplication between the Editable and this component but I suspect we'll diverge more as soon as we start adding features to this block

@jasmussen
Copy link
Contributor

Yaay! Thank you James!

@BoardJames
Copy link
Author

I've added a few minor bug fixes related to setting react state correctly and wiring up the alignment buttons.
I'll merge this as version 1 and start on the text formats dropdown and link button in another pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants