Framework: split RichText component (part 1) #15212

ellatrix · 2019-04-26T14:29:37Z

Description

What?

This PR splits out general RichText logic from the block-editor package to the rich-text package. It leaves block related context. That's all it does!

Why?

A RichText component can be used outside of the block or post context, such as in a sidebar, note...
It's easier to maintain.

How?

There will be two RichText components in each package, where the one in the block-editor package wraps the one in the rich-text package and adds block context.

There's a few temporary unstable props added that aid this wrapping, but in the future these props should be stabilised. Some props might disappears, some need to be renamed for the "bare" RichText component.

How has this been tested?

Nothing should have changed. Just ensure all e2e tests pass.

Screenshots

Types of changes

Refactoring.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

packages/rich-text/src/component/README.md

ellatrix · 2019-06-17T17:58:29Z

@hypest Do you foresee any problems with mobile? Who could help check?

packages/rich-text/README.md

hypest · 2019-06-19T06:32:42Z

Who could help check?

@SergioEstevao has already picked this up from the native mobile side 👍

hypest · 2019-06-19T06:40:00Z

👋 @ellatrix , I wonder, can you expand the PR description to include explanation/mention of the non-obvious changes in the PR? For example, this change from controls.map to [ 'bold', 'italic', 'link' ].map is not obvious and would be helpful to describe for the reader to understand. Such descriptions tend to be super useful when trying to debug issues later on.

packages/rich-text/src/component/editable.js

youknowriad · 2019-06-25T15:28:13Z

packages/block-editor/src/components/rich-text/index.js

+			__unstableOnEnterFormattedText={ onEnterFormattedText }
+			__unstableOnExitFormattedText={ onExitFormattedText }
+			__unstableCanUserUseUnfilteredHTML={ canUserUseUnfilteredHTML }
+			__unstableOnCreateUndoLevel={ onCreateUndoLevel }


Seems like a lot of unstable Props :) How do we plan to solve these in the future?

Yes! :) But I don't want to rewrite it all at once so I temporarily marked them unstable.

youknowriad · 2019-06-25T15:30:01Z

packages/block-editor/src/components/rich-text/index.js

+							<FormatToolbar />
+						</IsolatedEventContainer>
+					) }
+					{ isSelected && <RemoveBrowserShortcuts /> }


Why aren't these absorb by the underlying component (toolbars and RemoveBrowserShortcuts)?

It's block editor specific?

I'm not sure what RemoveBrowserShortcuts is but don't we want the format toolbar support by default in the RichText component?

It has toolbar support, but not a specific toolbar. This is the toolbar implementation for RichText instances in blocks. When using RichText (without the context), you could create your own custom toolbars.

youknowriad · 2019-06-25T15:32:52Z

packages/block-editor/src/components/rich-text/index.js

-	format: 'string',
-	value: '',
-};
-
 const RichTextContainer = compose( [
 	withInstanceId,
 	withBlockEditContext( ( { clientId } ) => ( { clientId } ) ),


Minor: The withSelect here, could become a useSelect

Yes, I've merely moved this code, I don't want to do a rewrite here to avoid making the PR more complex. I'm hoping for a quick merge so I don't have to do painful rebases. :)

youknowriad · 2019-06-25T15:35:22Z

packages/rich-text/package.json

@@ -22,11 +22,21 @@
 	"react-native": "src/index",
 	"dependencies": {
 		"@babel/runtime": "^7.4.4",
+		"@wordpress/blob": "file:../blob",


These changes make me think we need to update Core to support the webpack plugin to extract the dependencies, otherwise, we could miss these changes once we update Core. cc @gziolo @sirreal

Sorry for overlooking this. Is there an open issue somewhere to track this or can you provide more details? I don't know what would be involved in that work.

What's different here from other dependencies being added to packages?

youknowriad

Left some small comments, but this makes sense to me.

ellatrix · 2019-06-25T16:00:08Z

Thanks for the review @youknowriad! I will now merge the mobile PR into this one, then wait for tests to pass, then merge it into master.

hypest · 2019-06-25T16:10:45Z

then wait for tests to pass, then merge it into master

👋 @ellatrix , what do you think about #16219, can we have that updated first?

ellatrix · 2019-06-25T16:14:47Z

@hypest yes, that’s what I mean, the mobile PR needs to be merged into this branch?

hypest · 2019-06-25T16:18:14Z

#16219 seems to have conflicts currently. Are you planning to update it to resolve @ellatrix?

ellatrix · 2019-06-25T16:44:36Z

If no one beats me to it. I’ll have to do it later tonight.

…

On Tue, 25 Jun 2019 at 18:18, Stefanos Togoulidis ***@***.***> wrote: #16219 <#16219> seems to have conflicts currently. Are you planning to update it to resolve @ellatrix <https://github.com/ellatrix>? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15212?email_source=notifications&email_token=ABD6B25ISLG7X237IKGZWJTP4JANXA5CNFSM4HIWDL6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYQZHRA#issuecomment-505516996>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABD6B22ZU3Y7YBKFCQMF4JTP4JANXANCNFSM4HIWDL6A> .

hypest · 2019-06-25T16:48:43Z

I’ll have to do it later tonight

Thanks! I think I'll be able to help tomorrow if not done by then. Sérgio is AFK so, happy to help.

* Move React Native RichText to new package * Fix multiline reading. * Move default styles to component. * Move tests to component. * Fix paste on title

ellatrix · 2019-06-25T21:13:10Z

Thanks everyone!

youknowriad · 2019-06-26T07:39:25Z

packages/block-editor/src/components/rich-text/format-toolbar/index.js

 	return (
 		<div className="editor-format-toolbar block-editor-format-toolbar">
 			<Toolbar>
-				{ controls.map( ( format ) =>
+				{ [ 'bold', 'italic', 'link' ].map( ( format ) =>


Forgot to ask, why is this hardcoded now?

Ah, good question! These names are kind of hardcoded already in the formatting library. You cannot really add anything else here. But I forgot about the case where you'd remove the controls by setting an empty array. That case was already only partially working in master, see #14542. Let's try to correct it there again.

Now fixed in #14542.

ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text labels Apr 26, 2019

ellatrix requested review from aduth, chrisvanpatten, gziolo, jorgefilipecosta and youknowriad as code owners April 26, 2019 14:29

ellatrix force-pushed the try/move-rich-text branch from baac748 to 592246e Compare April 26, 2019 16:05

ellatrix mentioned this pull request Apr 26, 2019

Overview: RichText, APIs, & Formatting Library #13778

Closed

17 tasks

ellatrix added the [Status] In Progress Tracking issues with work in progress label Apr 26, 2019

ellatrix mentioned this pull request May 28, 2019

RichText: Add (foot)notes #15267

Closed

5 tasks

ellatrix force-pushed the try/move-rich-text branch from 8f58d2b to c6acc3b Compare June 7, 2019 09:55

ellatrix requested review from daniloercoli and SergioEstevao as code owners June 7, 2019 09:55

ellatrix force-pushed the try/move-rich-text branch from c6acc3b to ecf2816 Compare June 7, 2019 10:17

koke reviewed Jun 7, 2019

View reviewed changes

packages/rich-text/src/component/README.md Outdated Show resolved Hide resolved

ellatrix force-pushed the try/move-rich-text branch 3 times, most recently from d57587b to 068c5e4 Compare June 9, 2019 21:29

ellatrix force-pushed the try/move-rich-text branch from 068c5e4 to 2716e5f Compare June 17, 2019 19:48

ellatrix requested review from ajitbohra, nerrad, ntwb and talldan as code owners June 17, 2019 20:37

ellatrix added this to the Gutenberg 6.0 milestone Jun 18, 2019

ellatrix commented Jun 18, 2019

View reviewed changes

packages/rich-text/README.md Outdated Show resolved Hide resolved

SergioEstevao mentioned this pull request Jun 19, 2019

Move rich text native to separate package #16219

Merged

5 tasks

Add inline doc

6c3528f

ellatrix force-pushed the try/move-rich-text branch from b6505d0 to 6c3528f Compare June 25, 2019 14:47

Move doc block to export statement

02946d5

ellatrix changed the title ~~Framework: move RichText component to rich-text package~~ Framework: split RichText component (part 1) Jun 25, 2019

youknowriad reviewed Jun 25, 2019

View reviewed changes

packages/rich-text/src/component/editable.js Outdated Show resolved Hide resolved

youknowriad reviewed Jun 25, 2019

View reviewed changes

youknowriad approved these changes Jun 25, 2019

View reviewed changes

Do not mutate props

302fde5

Move rich text native to separate package (#16219)

140d0a1

* Move React Native RichText to new package * Fix multiline reading. * Move default styles to component. * Move tests to component. * Fix paste on title

ellatrix merged commit ebd94e2 into master Jun 25, 2019

ellatrix deleted the try/move-rich-text branch June 25, 2019 21:12

youknowriad reviewed Jun 26, 2019

View reviewed changes

youknowriad mentioned this pull request Jun 26, 2019

fix image block regression #16295

Merged

This was referenced Jun 26, 2019

RichText: add withoutInteractiveFormatting prop #14542

Merged

RichText: fix inline toolbar position #16299

Merged

afercia mentioned this pull request Sep 11, 2019

RichText changes and custom blocks backwards compatibility for WP 5.3 #17405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: split RichText component (part 1) #15212

Framework: split RichText component (part 1) #15212

ellatrix commented Apr 26, 2019 •

edited

ellatrix commented Jun 17, 2019

hypest commented Jun 19, 2019

hypest commented Jun 19, 2019

youknowriad Jun 25, 2019

ellatrix Jun 25, 2019

youknowriad Jun 25, 2019

ellatrix Jun 25, 2019

youknowriad Jun 25, 2019

ellatrix Jun 25, 2019

youknowriad Jun 25, 2019

ellatrix Jun 25, 2019

youknowriad Jun 25, 2019

sirreal Aug 20, 2019

ellatrix Aug 20, 2019

youknowriad left a comment

ellatrix commented Jun 25, 2019

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019 via email

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019

youknowriad Jun 26, 2019

ellatrix Jun 26, 2019

ellatrix Jun 26, 2019

Framework: split RichText component (part 1) #15212

Framework: split RichText component (part 1) #15212

Conversation

ellatrix commented Apr 26, 2019 • edited

Description

What?

Why?

How?

How has this been tested?

Screenshots

Types of changes

Checklist:

ellatrix commented Jun 17, 2019

hypest commented Jun 19, 2019

hypest commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

ellatrix commented Jun 25, 2019

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019 via email

hypest commented Jun 25, 2019

ellatrix commented Jun 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 26, 2019 •

edited