[Try] Customizable toolbar contents #23613
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { TransitionGroup, CSSTransition } from 'react-transition-group'; | ||
| import { throttle } from 'lodash'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useRef, useState, useEffect, useCallback } from '@wordpress/element'; | ||
| import warning from '@wordpress/warning'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import BlockControls from '../block-controls'; | ||
|
|
||
| export default function CustomizableBlockToolbarContent( { | ||
| children, | ||
| className, | ||
| } ) { | ||
| return ( | ||
| <BlockControls.Slot __experimentalIsExpanded> | ||
| { ( fills ) => { | ||
| return ( | ||
| <CustomizableBlockToolbarContentChildren | ||
| className={ className } | ||
| fills={ fills } | ||
| > | ||
| { children } | ||
| </CustomizableBlockToolbarContentChildren> | ||
| ); | ||
| } } | ||
| </BlockControls.Slot> | ||
| ); | ||
| } | ||
|
|
||
| function CustomizableBlockToolbarContentChildren( { | ||
| fills, | ||
| className = '', | ||
| children, | ||
| } ) { | ||
| const containerRef = useRef(); | ||
| const fillsRef = useRef(); | ||
| const toolbarRef = useRef(); | ||
| const [ dimensions, setDimensions ] = useState( {} ); | ||
|
|
||
| const fillsPropRef = useRef(); | ||
| fillsPropRef.current = fills; | ||
| const resize = useCallback( | ||
|
There was a problem hiding this comment. I think maybe we should name this to |
||
| throttle( ( elem ) => { | ||
| if ( ! elem ) { | ||
| elem = fillsPropRef.current.length | ||
| ? fillsRef.current | ||
| : toolbarRef.current; | ||
| } | ||
| if ( ! elem ) { | ||
| return; | ||
| } | ||
| elem.style.position = 'absolute'; | ||
| elem.style.width = 'auto'; | ||
| const css = window.getComputedStyle( elem, null ); | ||
|
There was a problem hiding this comment. Like the |
||
| setDimensions( { | ||
| width: css.getPropertyValue( 'width' ), | ||
| height: css.getPropertyValue( 'height' ), | ||
| } ); | ||
| elem.style.position = ''; | ||
| elem.style.width = ''; | ||
| }, 100 ), | ||
| [] | ||
| ); | ||
|
|
||
| useEffect( () => { | ||
| // Create an observer instance linked to the callback function | ||
|
There was a problem hiding this comment. This comment is describing what the next line does, we can remove it |
||
| const observer = new window.MutationObserver( function ( | ||
| mutationsList | ||
| ) { | ||
| const hasChildList = mutationsList.find( | ||
| ( { type } ) => type === 'childList' | ||
| ); | ||
| if ( hasChildList ) { | ||
| resize(); | ||
| } | ||
| } ); | ||
|
|
||
| // Start observing the target node for configured mutations | ||
|
There was a problem hiding this comment. same, this comment is describing what the next line does, we can remove it |
||
| observer.observe( containerRef.current, { | ||
| childList: true, | ||
| subtree: true, | ||
| } ); | ||
|
|
||
| return () => observer.disconnect(); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| if ( fills.length > 1 ) { | ||
| warning( | ||
| `${ fills.length } <BlockControls isExpanded> slots were registered but only one may be displayed.` | ||
| ); | ||
| } | ||
| }, [ fills.length ] ); | ||
|
|
||
| return ( | ||
| <div | ||
| className="block-editor-block-toolbar-width-container" | ||
| ref={ containerRef } | ||
| style={ dimensions } | ||
| > | ||
| <TransitionGroup> | ||
| { fills.length ? ( | ||
| <CSSTransition | ||
| key="fills" | ||
| timeout={ 300 } | ||
| classNames="block-editor-block-toolbar-content" | ||
| > | ||
| <div className={ className } ref={ fillsRef }> | ||
| { fills[ 0 ] } | ||
|
There was a problem hiding this comment. Nit pick, maybe make a |
||
| </div> | ||
| </CSSTransition> | ||
| ) : ( | ||
| <CSSTransition | ||
| key="default" | ||
| timeout={ 300 } | ||
| classNames="block-editor-block-toolbar-content" | ||
| > | ||
| <div className={ className } ref={ toolbarRef }> | ||
| { children } | ||
| </div> | ||
| </CSSTransition> | ||
| ) } | ||
| </TransitionGroup> | ||
| </div> | ||
| ); | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you had to use a different slot name though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting to use the exact same slot and fills but have some logic on the wrapper components to avoid rendering if there is already an expanded fill on the slot or something like that. Would that be possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How else would I make sure that expanded fills don't land inside the regular slot OR keep the non-expanded ones from replacing the entire content?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried inspecting the list of fills and looking at the
isExpandedprop, but then I realized it's just reinventing the slot name.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something's wrong with GitHub, I only saw your second comment a few minutes after posting mine.
I agree this would be the perfect solution. At the moment accessing fills outside of the slot is not possible and some refactoring would be required first so I went for the "wrapping slot" approach. Let me see what would it take to make it happen, it seems like virtually bubbling slots have a hook so maybe it wouldn't be that hard after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to trying to get rid of the
buildSlotName, as it implies there are countless options, when in fact we have twoThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm not sure either whether it's better or not, the current implementation is also decent. It's just that it feels more conceptually correct. (shouldn't block this PR though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small refactoring of bubbles-virtually/fill would make it possible to detect the presence of expanded fills outside of slot:
Before:
gutenberg/packages/components/src/slot-fill/bubbles-virtually/fill.js
Lines 16 to 18 in 9f2ca8b
After:
With that in place, the following would be possible:
So far so good - so we can use different logic for both cases. The problem is rendering only expanded or non-expanded fills here:
We don't have a granular control over fills at the slot level -
bubblesVirtuallyfill renders itself using portals. So either:So we're back to square one in regard to using slots and fills names. There would be a small win here in that we could get rid of the wrapping slot and make it a sibling of the default content instead, but it would still be a separate slot.