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

Rewire patterns plugin #1674

Merged
merged 7 commits into from
Jul 26, 2017
Merged

Rewire patterns plugin #1674

merged 7 commits into from
Jul 26, 2017

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jul 3, 2017

This PR aims to bring the core patterns plugin to Gutenberg. It's still a WIP, but some patterns already work:

  • Create a new text block and type . You get a list.
  • Prepend to a text block. It will be converted into a list.
  • Write `something` and it will be wrapped in <code>.
  • Wrap a word in ` and it will be wrapped in <code>.

* @return {String} Escaped characters
*/
function escapeRegExp( string ) {
return string.replace( /[\-\[\]\/\{\}\(\)\*\+\?\.\\\^\$\|]/g, '\\$&' );
Copy link
Member

Choose a reason for hiding this comment

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

you may find lodash.escapeRegExp incredibly handy. I sure have

@dmsnell
Copy link
Member

dmsnell commented Jul 3, 2017

Is this only for the TinyMCE blocks? I though that part of what we were getting away from is these awkward patterns. Like, if you want code, make a code block…

Can we write text that starts a line with * ?

@ellatrix
Copy link
Member Author

ellatrix commented Jul 3, 2017

Why are they awkward?

Regarding code, that one is inline, but I think also the lists are incredibly helpful.

@dmsnell
Copy link
Member

dmsnell commented Jul 3, 2017

Why are they awkward?

Well just because they aren't expected from the perspective of editing text. If you do this in Word or Google Docs or Pages or what-not they won't transform. It's just a question of whether we carry these forward or encourage the new semantics.

For example, what is the difference between an implicit list in the text block vs an explicit list block? Or, are we converting all these into separate blocks as soon as we type them?

@ellatrix
Copy link
Member Author

ellatrix commented Jul 3, 2017

They are converted into proper blocks. Google Docs, Pages, etc. are also all doing this.

@ellatrix
Copy link
Member Author

ellatrix commented Jul 3, 2017

I'm not sure if I understand your concern on a semantic level.

@brentjett
Copy link

Dropbox Paper has a nice implementation of these as well and they're pretty nice to use.

@ellatrix
Copy link
Member Author

ellatrix commented Jul 3, 2017

@dmsnell This PR is one of the reasons I was hoping to redo the editor state, so implementing these patterns would be easier and cleaner (without digging in the DOM). Mentioned in #703 (comment), #847 and #771.

];
const [ enterPatterns, spacePatterns ] = partition( patterns, ( { regExp } ) =>
regExp.source.endsWith( '$' )
);
Copy link
Member

Choose a reason for hiding this comment

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

nice!

although it's awkward to me splitting the function like this. was it too long for one line? I rarely split the arguments ( { regExp } ) from the function body via newline but that's personal style

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I should add {}. :)

Copy link
Member

Choose a reason for hiding this comment

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

it was more about splitting the function up when it's only a single line. all personal style

 = partition(
	patterns,
	( { regExp } ) => regExp.source.endsWith( '$' )
);

No more will I speak of this 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no strong opinions on style. More one or two but... :)

@ellatrix ellatrix force-pushed the try/editable-patterns branch from 8dd67ac to 22c1815 Compare July 8, 2017 10:58
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 22c1815 on try/editable-patterns into ** on master**.

@ellatrix
Copy link
Member Author

ellatrix commented Jul 8, 2017

@coveralls Could you elaborate?

@felixarntz
Copy link
Member

@iseulde I opened #1843 for discussing further Markdown syntax to support. I think the different # for headings plus links would be very beneficial to have in order to be able to quickly write regular post content. Not sure if this belongs here or into a separate ticket / PR.

@ellatrix
Copy link
Member Author

@felixarntz We can easily add the heading shortcuts here. The rest that is in core has already been added.

const transformsFrom = get( blockType, 'transforms.from', [] );
const transforms = transformsFrom.filter( ( { type } ) => type === 'pattern' );
return [ ...acc, ...transforms ];
}, [] );
Copy link
Member

Choose a reason for hiding this comment

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

nice use of a reducer here!

@mtias mtias added this to the Beta 0.6.0 milestone Jul 12, 2017
@mtias mtias added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Jul 12, 2017
@ellatrix ellatrix force-pushed the try/editable-patterns branch from 22c1815 to 3215b68 Compare July 18, 2017 12:21
@ellatrix
Copy link
Member Author

ellatrix commented Jul 20, 2017

Not sure how I should bring over the tests from PhantomJS + QUnit here. Any ideas? Core test file: https://github.com/WordPress/wordpress-develop/blob/master/tests/qunit/wp-includes/js/tinymce/plugins/wptextpattern/plugin.js

@ellatrix ellatrix added the [Status] In Progress Tracking issues with work in progress label Jul 20, 2017
@ellatrix ellatrix self-assigned this Jul 20, 2017
@ellatrix ellatrix added the [Feature] Block API API that allows to express the block paradigm. label Jul 20, 2017
@ellatrix
Copy link
Member Author

@nylen What are your thoughts on the tests? I think we should either go ahead and merge this to move forward, or try to add the QUnit tests and manually run them for this.

@ellatrix ellatrix force-pushed the try/editable-patterns branch from 44d9d35 to e214fe2 Compare July 26, 2017 18:54
@ellatrix
Copy link
Member Author

Let’s merge and iterate. ✨

@ellatrix ellatrix merged commit 2314faa into master Jul 26, 2017
@ellatrix ellatrix deleted the try/editable-patterns branch July 26, 2017 18:55
@nylen
Copy link
Member

nylen commented Jul 26, 2017

I've been exploring using https://webdriver.io/ to write some integration tests, and I have the basic structure working. This is a much heavier approach than core's existing QUnit tests, but it has the benefit of being able to test block splitting, focus behavior, etc as well as simply the patterns and selection features within a contenteditable. It also offers some interesting possibilities for automated testing in real browsers with BrowserStack or other similar services.

@ellatrix
Copy link
Member Author

That sounds good to me, if it's abstracted and reusable. Do you think this will slow the tests down a lot? Should we run it only when certain files change? In any case, let's explore in a separate PR.

@nylen
Copy link
Member

nylen commented Jul 26, 2017

Yes, they are slower than the rest of our tests. We should probably only run them against master.

@mtias mtias mentioned this pull request Jul 28, 2017

const [ enterPatterns, spacePatterns ] = partition(
patterns,
( { regExp } ) => regExp.source.endsWith( '$' ),
Copy link
Member

Choose a reason for hiding this comment

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

ES2015 string prototypes methods aren't polyfilled, so this will error in IE11.

See also: #746

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks 🙈

}

const block = pattern.transform( {
content: [ remainingText, ...drop( content ) ],
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to track down what this transform function is meant to accept as an argument. Is the content object intended to be an array of two members, the first entry the text following the matching and the second entry the matched substring? Is it then expected that any pattern must match only at the start of a string? Where is this documented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants