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

[RNMobile] Split Para/Heading blocks on enter.KEY - step 1 #10553

Merged
merged 7 commits into from
Oct 16, 2018

Conversation

daniloercoli
Copy link
Contributor

This PR does add a very simple implementation of onHTMLContentWithCursor method to RichText. The code doesn't check much at the moment, but just call onSplit on the outer Block, if it has onSplit defined.

onSpliton both Para and Heading blocks, is very minimal, it just calls insertAfter without checking the position of the cursor, or any other condition.

…omponent, when enter.KEY is detected.

Para block for mobile now has onSplit defined, that in this first implementation tries to add a test  block after.
@daniloercoli daniloercoli added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Oct 12, 2018
@@ -24,6 +24,25 @@ import './editor.scss';
const minHeight = 50;

class HeadingEdit extends Component {
constructor() {
super( ...arguments );

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: can we remove this extra line here?


splitContent( htmlText, start, end ) {
const { onSplit } = this.props;

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick #2: can we remove this extra line here?

import { RichText } from '@wordpress/editor';

const minHeight = 50;

class ParagraphEdit extends Component {
constructor() {
super( ...arguments );

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick #3: let's remove the extra line here

// Descriptive placeholder: This logic still needs to be implemented.
onHTMLContentWithCursor( htmlText, start, end ) {
if ( ! this.props.onSplit ) {
// insert the \n char instead?
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a ToDo comment here to make sure this is something we come back to at a follow up stage

@daniloercoli
Copy link
Contributor Author

Ready for another round @mzorz! 🙇

}

// eslint-disable-next-line no-unused-vars
splitBlock( htmlText, start, end ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@daniloercoli , can we perhaps go closer to the interface of the web-side counterpart, wdyt? https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/heading/edit.js#L52.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, we will probably go toward that direction in the short future. For now i'd like to go with the current implementation, and get the ball rolling.

Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the small-ish comments! LGTM :shipit:

@daniloercoli daniloercoli merged commit a9a27bb into master Oct 16, 2018
@daniloercoli daniloercoli deleted the rnmobile/split-para-on-enter branch October 16, 2018 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants