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 findDOMNode usage from the inserter #11363

Merged
merged 2 commits into from
Nov 1, 2018
Merged

Remove findDOMNode usage from the inserter #11363

merged 2 commits into from
Nov 1, 2018

Conversation

youknowriad
Copy link
Contributor

Related #11360

This PR leverages forwardRef to remove findDOMNode usage from the inserter. The idea is that you can ref a PanelBody component and you get the inner div instead. It's a small breaking change but since we never documented ref as a public prop for PanelBody I don't expect this to break anything.

@youknowriad youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Nov 1, 2018
@youknowriad youknowriad self-assigned this Nov 1, 2018
@youknowriad youknowriad requested a review from a team November 1, 2018 14:13
@youknowriad youknowriad mentioned this pull request Nov 1, 2018
Closed
12 tasks
jorgefilipecosta
jorgefilipecosta reviewed Nov 1, 2018
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @youknowriad in my tests this is working correctly 👍
Normally we use createRef() to initilize the references would this apporach be equivalent:
image

jorgefilipecosta
jorgefilipecosta reviewed Nov 1, 2018
View reviewed changes
packages/editor/src/components/panel-color-settings/test/__snapshots__/index.js.snap
@@ -87,7 +87,7 @@ exports[`PanelColorSettings should render a color panel if at least one setting
`;

exports[`PanelColorSettings should render a color panel if at least one setting specifies some colors to choose 2`] = `
<PanelBody
<[object Object]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why the displayName we set is not being used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably an enzyme bug :)

@youknowriad
Copy link
Contributor Author

@jorgefilipecosta I don't know if it's equivalent. In general the createRef is intialized in the constructor while in this case we don't know all the instances we want to create.

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta I don't know if it's equivalent. In general the createRef is intialized in the constructor while in this case we don't know all the instances we want to create.

That's true, nothing says it must be initialized in the constructor but your approach seems safer.

jorgefilipecosta
jorgefilipecosta approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@youknowriad youknowriad merged commit 6919650 into master Nov 1, 2018
@youknowriad youknowriad deleted the remove/find-dom-node-inserter branch November 1, 2018 17:32
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
@jorgefilipecosta jorgefilipecosta mentioned this pull request Nov 1, 2018
Merged
@youknowriad youknowriad added this to the 4.3 milestone Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorgefilipecosta jorgefilipecosta jorgefilipecosta approved these changes

Assignees

@youknowriad youknowriad

Labels
[Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@youknowriad @jorgefilipecosta