Skip to content

Manage Editable value as React element #466

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.

Sign up for GitHub

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

Merged
aduth merged 5 commits into from
Apr 24, 2017
Merged

Manage Editable value as React element #466

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d36e98
Add children query matcher using HtmlToReact
aduth Apr 20, 2017
479d01d
Enhance renderToString to handle more incoming value types
aduth Apr 20, 2017
fbae376
Manage Editable value as React element
aduth Apr 20, 2017
2c40c99
Parse Editable content as raw
aduth Apr 20, 2017
82feb24
Use React.Children to map content
aduth Apr 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Use React.Children to map content 
Designed for this purpose of handling opaque structure of children

https://facebook.github.io/react/docs/react-api.html#react.children
  • Loading branch information
@aduth
aduth committed Apr 24, 2017
commit 82feb249cb1dad23bc93088d48f5457a4762283d
11 changes: 2 additions & 9 deletions blocks/library/text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,26 +71,19 @@ registerBlock( 'core/text', {
save( { attributes } ) {
// An empty block will have an undefined content field. Return early
// as an empty string.
let { content } = attributes;
const { content } = attributes;
if ( ! content ) {
return '';
}

// Content can be in the following shapes, so we normalize to an array:
// - Single paragraph: Object
// - Multiple paragraph: Array of objects
if ( ! Array.isArray( content ) ) {
content = [ content ];
}

// We only need to transform content if we need to apply the alignment
// style. Otherwise we can return unmodified.
const { align } = attributes;
if ( ! align ) {
return content;
}

return content.map( ( paragraph ) => (
return wp.element.Children.map( content, ( paragraph ) => (
wp.element.cloneElement( paragraph, {
style: { textAlign: align }
} )
Expand Down
4 changes: 3 additions & 1 deletion element/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { createElement, Component, cloneElement } from 'react';
import { createElement, Component, cloneElement, Children } from 'react';
import { render } from 'react-dom';
import { renderToStaticMarkup } from 'react-dom/server';

Expand Down Expand Up @@ -40,6 +40,8 @@ export { Component };
*/
export { cloneElement };
Copy link
Contributor

@youknowriad youknowriad Apr 21, 2017

Choose a reason for hiding this comment

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

Honestly, I don't see much value in abstracting "React"

Copy link
Member Author

@aduth aduth Apr 21, 2017

Choose a reason for hiding this comment

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

Honestly, I don't see much value in abstracting "React"

What are you suggesting here?

Copy link
Contributor

@youknowriad youknowriad Apr 21, 2017

Choose a reason for hiding this comment

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

we just drop wp.element :)

Copy link
Member Author

@aduth aduth Apr 21, 2017

Choose a reason for hiding this comment

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

I could probably be convinced. Needs to be a decision made with future compatibility in mind, related to some of the "reason to be" outlined in the docs:

https://github.com/WordPress/gutenberg/tree/master/element#element

Copy link
Contributor

@youknowriad youknowriad Apr 21, 2017

Choose a reason for hiding this comment

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

To clarify my point, I don't see much value in abstracting React because we're using a lot of React features: Element, Component (refs, state, lifecycle), cloneElement, renderToString, render. Which is close to be the whole React.

Also, and this is probably the main reason for me. If we want to use a library like react-textarea-autosize or react-tooltip or any library dependent on React, we can do so (we're already doing it) technically and it'll work because we're using React but what if we switch our wp.element implementation to a custom React alternative, nothing guarantees that the libraries we're using are only using the React features we whitelist on our implementation. And this IMO should force us to avoid using any React third-party library because "logically" they're not dependent on our abstraction but they're dependent on React (which is a larger library).


export { Children };

/**
* Renders a given element into a string
*
Expand Down