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.

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

Jump to bottom

Remove deprecated componentWillReceiveProps from TinyMCE component #11368

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Remove deprecated componentWillReceiveProps from TinyMCE component #11368

merged 1 commit into from
Nov 1, 2018

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 1, 2018

Description

See #11360. Removes componentWillReceiveProps by doing all updates during shouldComponentUpdate. Normally this should only return boolean without side effects, but since we're doing our own reconciliation within this component, I think it's fine to use shouldComponentUpdate to figure out what should be updated.

@aduth @youknowriad Do you think this is fine?

How has this been tested?

Make sure placeholders display correctly, aria props, className and styles update.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@ellatrix ellatrix force-pushed the remove/tinymce-component-will-receive-props branch from 4610cef to 47fac11 Compare November 1, 2018 16:19
@youknowriad youknowriad mentioned this pull request Nov 1, 2018
Closed
12 tasks
aduth
aduth approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

This feels wrong for obvious reasons, but I think it highlights how the role of the TinyMCE component has ballooned from its original purpose and does more than it probably should, and that React doesn't really otherwise provide support for reconciliation of contentEditable fields.

I might like to see some inline comment which informs the future maintainer to this rationale for breaking best practices in introducing side effects to shouldComponentUpdate.

@aduth
Copy link
Member

aduth commented Nov 1, 2018

Supersedes #8148

@aduth aduth mentioned this pull request Nov 1, 2018
Closed
@ellatrix
Copy link
Member Author

ellatrix commented Nov 1, 2018 •
edited
Loading

Before doing this, I tried a number of other things, including making a subcomponent that would just be RawHTML with shouldComponentUpdate set to false. This would work fine I think, if RawHTML didn't include the wrapper div. It would be nice if there was React support for this, e.g. what is proposed here: facebook/react#12014 (comment). That would be RawHTML without rendering any wrappers.

Either that, or I wish React would simply ignore contentEditable children entirely (with or without prop to enable).

@ellatrix ellatrix merged commit 9c8923a into master Nov 1, 2018
@ellatrix ellatrix deleted the remove/tinymce-component-will-receive-props branch November 1, 2018 17:28
@ellatrix
Copy link
Member Author

ellatrix commented Nov 1, 2018

I might like to see some inline comment which informs the future maintainer to this rationale for breaking best practices in introducing side effects to shouldComponentUpdate.

@aduth Sorry, I read over the last line. :/ Shall I make a follow-up PR?

daniloercoli added a commit that referenced this pull request Nov 1, 2018
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
4e7dd62 
…rnmobile/port-quote-block-step-1

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Add removed periods to block descriptions. (#11367)
  Remove findDOMNode usage from the inserter (#11363)
  Remove deprecated componentWillReceiveProps from TinyMCE component (#11368)
  Create file blocks when dropping multiple files at once (#11297)
  Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168)
  Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180)
  Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342)
  Remove the Cloudflare warning (#11350)
  Image Block: Use source_url for media file link (#11254)
  Enhance styling of nextpage block using the Hr element (#11354)
  Embed block refactor and tidy (#10958)
  Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347)
  Fix RTL block alignments (#11293)
  RichText: fix buggy enter/delete behaviour (#11287)
  Remove code coverage setup (#11198)
  Parser: Runs all parser implementations against the same tests (#11320)
  Stop trying to autosave when title and classic block content both are empty. (#10404)
  Fix "Mac OS" typo + use fancy quotes consistently (#11310)
  Update documentation link paths (#11324)
  Editor: Reshape blocks state under own key (#11315)
  ...

# Conflicts:
#	gutenberg-mobile
@aduth
Copy link
Member

aduth commented Nov 1, 2018

It'd be good to have. I'd also not mind a direct-to-master commit if all it contains is a code comment.

@youknowriad youknowriad added this to the 4.3 milestone Nov 2, 2018
@ellatrix ellatrix mentioned this pull request Nov 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduth aduth aduth approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
3 participants
@ellatrix @aduth @youknowriad