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

Tiny with react ui #275

Closed
wants to merge 459 commits into from
Closed

Tiny with react ui #275

wants to merge 459 commits into from

Conversation

mimo84
Copy link
Contributor

@mimo84 mimo84 commented Mar 15, 2017

Please see PR #294 which replaces this one.


Hi all, we have a prototype going for #229.
You can see it at https://intronic.github.io/gutenberg/tinymce-single-react-ui/

What is in the demo:

What works
Block Menu

  • clicking in a block will highlight the outer border of the block, and bring up a block-change menu icon at the top right.
  • clicking in different blocks brings up the menu for that block
  • hover over different blocks highlights that block with a vertical bar, but doesnt change the focus
  • hover over the block-change icon and the block-align menu will show
  • click the block-change icon and a drop-down of different block types will be shown
  • the block-menu icon changes depending on the type of block (currently only P/H/Blockquote - not IMG, etc)

Inline style Menu

  • the inline menu shows up only when a range is selected and it appears above the selected text (similar to medium)
  • the inline menu button states show the state of the highlighted text (eg, bold)
  • using the keyboard to toggle the highlight text bold or italic will also toggle the button states

Key difference in the code

  • UI actions, UI state transitions, and UI rendering, and styling are all decoupled so changing the UI is simpler
  • Most of the issues in Prototypes: Requirements #190 will be addressed once all the editor actions are linked up, as contenteditable is controlled by a single instance of TinyMCE

What is not completed yet

  • linking the button click handlers to the tiny commands (eg, none of the buttons are functional)
  • positioning the block highlight after scrolling the page
  • the original spec called for the inline menu to be black with white text to distinguish it from the block-menu, but it currently is still white background and black text
  • lots of other stuff

Cheers,

Mike, Maurizio & Anna

@iseulde @jasmussen @androb

ellatrix and others added 30 commits March 8, 2017 14:56
Normalize some of the single instance interface
Adjust inserting for heading and paragraph
Allow insertion by returning string or node
Remove blockquote should remove entire blockquote
Toggle lists to line breaks instead of paragraphs
…able-false

Use contenteditable false for object blocks
@mtias mtias added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Mar 15, 2017
</div>
}
<div>
<InlineToolbar isOpen={ inlineOpen(collapsed) }
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're building the UI in React, should we have a different Inline/Block toolbar depending on the current block ? Shoud this be part of the block API?

Copy link
Contributor

Choose a reason for hiding this comment

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

@youknowriad we split these two things to explore an idea of @annaephox.
So that inline editing options are be presented to the user directly at any range selection in the same way as medium, while leaving the block-level operations attached to the block.

We (@mimo84 and I) only just started on this project and didnt understand the blocks API requirement until this PR :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry. My question was ambiguous. I was asking how you each block defines each control and react to its controls. For now, you have a "global" inline toolbar that shows the controls no matter the selected block.

The same applies to the block toolbar.

blockRect={ rangeRect(topBlock) }
/>
</div>
<TinyMCEReact content={window.content}
Copy link
Contributor

Choose a reason for hiding this comment

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

How do the content get reflected in the state? My knowledge about Redux and State based architecture is that the state should be the source of truth and it should be rendered in the components.
I think the content prop here is never updated.

Copy link
Contributor

@intronic intronic Mar 16, 2017

Choose a reason for hiding this comment

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

This is where it starts:

https://github.com/WordPress/gutenberg/pull/275/files#diff-29319b526b66e55eb72a1e1696deee28

  • we create an app state 'store' and attach the 'actions' to it
  • the actions define the state transitions (state, action) -> (state)
  • we pass the state store through to the App so it can dispatch action messages to the store
  • the app subscribes to any state changes to the store

Then the Turducken component coordinates the UI and Tiny:

https://github.com/WordPress/gutenberg/pull/275/files#diff-aa23597862e3bf21c5f904c13462266a

  • it supplies state values as props to the *Toolbars (and the DIV outline blocks which we didnt have time to move out into its own component)
  • it supplies dispatch functions as props to the Tiny component which dispatch action messages to the store when content state that is relevant to the UI changes

Copy link
Contributor

Choose a reason for hiding this comment

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

A few other points:

  • redux is not tied to react (they point out Deku which I'd not seen : http://redux.js.org/docs/basics/UsageWithReact.html)
  • we could use other helper libraries like react-redux, rather than redux directly, which abstract things a bit and make the code cleaner to read, or terser, but a bit more opaque to understand (requires learning that specific library)
  • redux works like the ClojureScript approach to app state in things like Reagent & Om which I am more familiar with

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these clarifications, I'm already aware of how Redux and React works :), my question here was regarding the content prop. Should we make it "controlled" and "updated" when the state changes (I guess it's the same question as the controlled TinyMCE question down here)

if ( this.props.focusConfig ) {
this.editor.selection.moveToBookmark( bookmark );
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this code comes from the per-block prototype. My problem here is that I was not able to have a "controlled" TinyMCE, which means I was not able to call setContent each time the content prop changes without loosing the cursor position.

My second problem is that I was not able to get a reliable onChange event which is essential in a state based architecture. the TinyMCE's onChange is not triggered on each change and even when it was called, I think the content I got which editor.getContent was different from the real content at that moment.

Maybe you could have more expertise on these?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was really happy to use your code for the tiny-inside-react component, I think its really well done.
We only had 2 days to work on this prototype so I just hacked the tiny component to do the minimal changes we needed to get UI state from Tiny.

My main goal was to show how the UI and UI state can be treated separately from the content state even with the single tiny concept.

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding a 'controlled' component

  • we could try it out (can restore the selection, for example)
  • this would probably mean though that tiny plugins that change the dom will fail
  • it seems less intrusive and risky to drive it the unmanaged way you did:
    -- using the Tiny API - adding event handlers for state updates, and calling ExecCommand to get things done in the content

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I've started by trying a "controlled" approach which is closer to the React way of doing things but I failed, that's why I've used an uncontrolled TinyMCE with an imperative API.

node: null, // current node of the selection (if collapsed) or common ancestor containing the selection)
range: null, // current selection range
editorRef: null // reference to the editor
})
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we store the blocks on the store? I think this is the most important thing to have a good Separation of Concern. Having the ability to store the editor's content by block on the store and updating the UI each time this content is changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, these two things are inter-related but different, and we have some different constraints on them:

  • Document content state
  • UI state

Copy link
Contributor

Choose a reason for hiding this comment

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

storing the blocks in the store: depends on lots of things really

  • what we define as blocks
  • whether blocks exist in a hierarchy
  • assuming the content state is managed in the browser dom

In this Tiny-single example we just deduce the blocks using the existing block search code that looks up from the current selection up to the element which is a direct child of the body. - This implies blocks are a linear sequence.

In the Tiny-singe example we could also look up from the current selection to the nearest enclosing 'block' type element (what ever HTML5 tags or rules those are defined to be).

  • This would give a hierarchy of nested blocks following the dom structure
  • if we also stored those blocks in the state it would presumably need some sort of tree structure

@youknowriad
Copy link
Contributor

Thanks for the prototype @mimo84 @intronic I do think having a global contenteditable and a state based architecture is the best of two worlds (the two prototypes). But As I said in my comments, I think having the block contents in the state is the most important thing to achieve this. At its current state, this prototype have the selection and I think the current selected block on the state, but not the entire content organized by blocks which is the starting point of the Separation of Concerns.

Also I couldn't figure out how to define blocks (to be able to define custom blocks).

@intronic
Copy link
Contributor

Hi @youknowriad

This is an interesting point about the UI state and content state.

  • In this demo I thought it important to surface the UI state so its clear about how changes to the content affect the UI.

  • The content state is another thing entirely - there is a big constraint right now for either prototype model:
    -- right now the single source of truth of content state while editing is the live browser DOM (of the users choice), and it is persisted as HTML.

@youknowriad
Copy link
Contributor

For the multi-instance model, the single source of truth for all the state (UI and Content) is the editor state object (JS object https://github.com/WordPress/gutenberg/blob/master/tinymce-per-block/src/renderers/block/index.js#L18-L23 ) that could be moved to a Redux store https://github.com/WordPress/gutenberg/blob/master/tinymce-per-block/src/renderers/block/index.js#L18-L23 (I've just avoided another library) but you can see here that I've something similar to a "Reducer" https://github.com/WordPress/gutenberg/blob/master/tinymce-per-block/src/renderers/block/updater.js#L236 and here are the actions https://github.com/WordPress/gutenberg/blob/master/tinymce-per-block/src/renderers/block/commands.js

And here is the Data Flow diagram https://github.com/WordPress/gutenberg/blob/master/tinymce-per-block/doc/data-flow.png (the blue box is the source of truth we manipulate through time and it gets rendered with React)

I think if we can achieve a similar DataFlow with the single TinyMCE approach (means moving the content to state instead of relying on the DOM), we would have the best of the two worlds. I really think the UI state is less important than the content state.

@jasmussen
Copy link
Contributor

Seems like this came together pretty fast!

UI-wise it's pretty far from the UI baseline at https://github.com/WordPress/gutenberg#mockups. That's not a knock on how it looks — I like the minimalism of some of the block level controls revealing themselves on hovering or clicking the type indicator. But for the purposes of comparing the prototypes on their technical underpinnings rather than perceived UI benefits, this makes it difficult. So I'll belay commenting on omissions or divergences on the UI unless you'd like that specifically, then let me know.

@mimo84
Copy link
Contributor Author

mimo84 commented Mar 17, 2017

We decided to open a new PR #285 we can continue the discussion there.
@intronic @youknowriad @jasmussen @iseulde @mtias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants