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

Refactor fill component to avoid relying on componentWillUpdate and use React Hooks instead #15541

Merged
merged 2 commits into from
May 21, 2019
Merged

Refactor fill component to avoid relying on componentWillUpdate and use React Hooks instead #15541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
983d0a3
Refactor fill component to avoid relying on componentWillUpdate and u…
youknowriad May 9, 2019
7c40b02
Fix the reordering issue
youknowriad May 20, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/components/src/slot-fill/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,12 @@ class SlotFillProvider extends Component {
if ( this.slots[ name ] !== slotInstance ) {
return [];
}

return sortBy( this.fills[ name ], 'occurrence' );
}

resetFillOccurrence( name ) {
forEach( this.fills[ name ], ( instance ) => {
instance.resetOccurrence();
instance.occurrence = undefined;
} );
}

Expand Down
82 changes: 37 additions & 45 deletions packages/components/src/slot-fill/fill.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { isFunction } from 'lodash';
/**
* WordPress dependencies
*/
import { Component, createPortal } from '@wordpress/element';
import { createPortal, useLayoutEffect, useRef, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -15,64 +15,56 @@ import { Consumer } from './context';

let occurrences = 0;

class FillComponent extends Component {
constructor() {
super( ...arguments );
this.occurrence = ++occurrences;
}
function FillComponent( { name, getSlot, children, registerFill, unregisterFill } ) {
// Random state used to rerender the component if needed, ideally we don't need this
const [ , updateRerenderState ] = useState( {} );
const rerender = () => updateRerenderState( {} );
Copy link
Member

Choose a reason for hiding this comment

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

According to @gaearon

https://twitter.com/dan_abramov/status/1118301474876948480

you can do this:

const [, forceUpdate] = useReducer(x => x + 1, 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, not sure which one is best. I guess it doesn't matter that much


componentDidMount() {
const { registerFill } = this.props;
const ref = useRef( {
name,
children,
} );

registerFill( this.props.name, this );
if ( ! ref.current.occurrence ) {
ref.current.occurrence = ++occurrences;
}

componentWillUpdate() {
if ( ! this.occurrence ) {
this.occurrence = ++occurrences;
}
const { getSlot } = this.props;
const slot = getSlot( this.props.name );
useLayoutEffect( () => {
ref.current.forceUpdate = rerender;
registerFill( name, ref.current );
return () => unregisterFill( name, ref.current );
}, [] );

useLayoutEffect( () => {
ref.current.children = children;
const slot = getSlot( name );
if ( slot && ! slot.props.bubblesVirtually ) {
slot.forceUpdate();
}
}

componentWillUnmount() {
const { unregisterFill } = this.props;
}, [ children ] );

unregisterFill( this.props.name, this );
}
useLayoutEffect( () => {
if ( name === ref.current.name ) {
// ignore initial effect
return;
}
unregisterFill( ref.current.name, ref.current );
ref.current.name = name;
registerFill( name, ref.current );
}, [ name ] );

componentDidUpdate( prevProps ) {
const { name, unregisterFill, registerFill } = this.props;
const slot = getSlot( name );

if ( prevProps.name !== name ) {
unregisterFill( prevProps.name, this );
registerFill( name, this );
}
if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) {
return null;
}

resetOccurrence() {
this.occurrence = null;
// If a function is passed as a child, provide it with the fillProps.
if ( isFunction( children ) ) {
children = children( slot.props.fillProps );
}

render() {
const { name, getSlot } = this.props;
let { children } = this.props;
const slot = getSlot( name );

if ( ! slot || ! slot.node || ! slot.props.bubblesVirtually ) {
return null;
}

// If a function is passed as a child, provide it with the fillProps.
if ( isFunction( children ) ) {
children = children( slot.props.fillProps );
}

return createPortal( children, slot.node );
}
return createPortal( children, slot.node );
}

const Fill = ( props ) => (
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/slot-fill/slot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class SlotComponent extends Component {

const fills = map( getFills( name, this ), ( fill ) => {
const fillKey = fill.occurrence;
const fillChildren = isFunction( fill.props.children ) ? fill.props.children( fillProps ) : fill.props.children;
const fillChildren = isFunction( fill.children ) ? fill.children( fillProps ) : fill.children;

return Children.map( fillChildren, ( child, childIndex ) => {
if ( ! child || isString( child ) ) {
Expand Down