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

Rich Text: Deprecate event proxying #6286

Merged
merged 1 commit into from
May 1, 2018
Merged

Conversation

aduth
Copy link
Member

@aduth aduth commented Apr 19, 2018

This pull request seeks to deprecate the RichText component's proxyPropEvents function, which handles and propagates a full set of events bound to TinyMCE.

There are a few motivations for this change:

  • Performance. We are handling costly events such as mousemove for RichText.
  • Explicitness. Be very clear about what props are supported for RichText.
  • Consistency. Some callbacks like onChange have transforms applied in addition to the default event handling, and don't pass event as a callback argument.

This was originally introduced in the very first implementation of the autocompleter (#2630), where the autocompleter had applied props directly to the instance of RichText. This was refactored away in #2896.

Many alternatives exist, and are included in the deprecation message:

  • Documented props
  • Ancestor element event handler
  • onSetup and binding events directly to the internal editor instance

Note: This is not expected to have an impact on the majority usage of RichText. The onChange callback still behaves as it did previously.

Testing instructions:

Verify there are no regressions in the behavior of RichText.

Try adding an event like onContextMenu to a RichText element and verify that a deprecation warning occurs.

@aduth aduth added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts labels Apr 19, 2018
@aduth aduth changed the title Rich Text: Deprecate broad handling of events Rich Text: Deprecate event proxying Apr 19, 2018
@aduth aduth force-pushed the deprecate/rich-text-events branch from 6047024 to 4f49928 Compare May 1, 2018 20:01
@aduth aduth merged commit afa2195 into master May 1, 2018
@aduth aduth deleted the deprecate/rich-text-events branch May 1, 2018 20:07
@aduth aduth added this to the 2.8 milestone May 1, 2018
@aduth
Copy link
Member Author

aduth commented May 2, 2018

To clarify, this will start causing warnings on blocks which used the pattern for setting focus on a block via setFocus (i.e. onFocus={ setFocus }), made redundant in #5029 and then explicitly removed in #6352.

Such blocks should just remove onFocus props altogether, as they do not need to handle isSelected themselves. See also #5029.

@youknowriad
Copy link
Contributor

youknowriad commented May 18, 2018

I wonder if we should introduce an explicit onFocus prop, it seems to be something commonly used across blocks even after #5039

@youknowriad
Copy link
Contributor

So it looks like our remaining usage of onFocus is to ensure Image and Gallery captions show the toolbar of the captions when needed. I wonder if we can't automatically handle this similar to #5039 cc @gziolo

@gziolo
Copy link
Member

gziolo commented May 18, 2018

Yes, we can, needs more work on our side. I can schedule it for the next week.

@gziolo
Copy link
Member

gziolo commented May 21, 2018

@youknowriad, I opened #6871 to address the issue you raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants