-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Editor: Extract BlockList as reusable component #3546
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,8 +16,8 @@ import { __, sprintf } from '@wordpress/i18n'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import BlockMover from '../../components/block-mover'; | ||
import BlockSettingsMenu from '../../components/block-settings-menu'; | ||
import BlockMover from '../block-mover'; | ||
import BlockSettingsMenu from '../block-settings-menu'; | ||
import InvalidBlockWarning from './invalid-block-warning'; | ||
import BlockCrashWarning from './block-crash-warning'; | ||
import BlockCrashBoundary from './block-crash-boundary'; | ||
|
@@ -56,7 +56,31 @@ import { | |
|
||
const { BACKSPACE, ESCAPE, DELETE, ENTER, UP, RIGHT, DOWN, LEFT } = keycodes; | ||
|
||
class VisualEditorBlock extends Component { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Making this reusable means we need to use a prop or something to get a selector for the parent scrollable. https://github.com/WordPress/gutenberg/pull/3546/files#diff-bc66d5701a2fea1111bb8f1bf0150910R96 It's not guaranteed to be the same in all layouts. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In d8be96b I reimplemented the behavior to a generic "find closest scrollable container". |
||
/** | ||
* Given a DOM node, finds the closest scrollable container node. | ||
* | ||
* @param {Element} node Node from which to start | ||
* @return {?Element} Scrollable container node, if found | ||
*/ | ||
function getScrollContainer( node ) { | ||
if ( ! node ) { | ||
return; | ||
} | ||
|
||
// Scrollable if scrollable height exceeds displayed... | ||
if ( node.scrollHeight > node.clientHeight ) { | ||
// ...except when overflow is defined to be hidden or visible | ||
const { overflowY } = window.getComputedStyle( node ); | ||
if ( /(auto|scroll)/.test( overflowY ) ) { | ||
return node; | ||
} | ||
} | ||
|
||
// Continue traversing | ||
return getScrollContainer( node.parentNode ); | ||
} | ||
|
||
class BlockListBlock extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
|
||
|
@@ -91,9 +115,6 @@ class VisualEditorBlock extends Component { | |
if ( this.props.isTyping ) { | ||
document.addEventListener( 'mousemove', this.stopTypingOnMouseMove ); | ||
} | ||
|
||
// Not Ideal, but it's the easiest way to get the scrollable container | ||
this.editorLayout = document.querySelector( '.editor-layout__content' ); | ||
} | ||
|
||
componentWillReceiveProps( newProps ) { | ||
|
@@ -109,9 +130,13 @@ class VisualEditorBlock extends Component { | |
componentDidUpdate( prevProps ) { | ||
// Preserve scroll prosition when block rearranged | ||
if ( this.previousOffset ) { | ||
this.editorLayout.scrollTop = this.editorLayout.scrollTop + | ||
this.node.getBoundingClientRect().top - | ||
this.previousOffset; | ||
const scrollContainer = getScrollContainer( this.node ); | ||
if ( scrollContainer ) { | ||
scrollContainer.scrollTop = scrollContainer.scrollTop + | ||
this.node.getBoundingClientRect().top - | ||
this.previousOffset; | ||
} | ||
|
||
this.previousOffset = null; | ||
} | ||
|
||
|
@@ -324,7 +349,7 @@ class VisualEditorBlock extends Component { | |
const { isHovered, isSelected, isMultiSelected, isFirstMultiSelected, focus } = this.props; | ||
const showUI = isSelected && ( ! this.props.isTyping || ( focus && focus.collapsed === false ) ); | ||
const { error } = this.state; | ||
const wrapperClassName = classnames( 'editor-visual-editor__block', { | ||
const wrapperClassName = classnames( 'editor-block-list__block', { | ||
'has-warning': ! isValid || !! error, | ||
'is-selected': showUI, | ||
'is-multi-selected': isMultiSelected, | ||
|
@@ -369,7 +394,7 @@ class VisualEditorBlock extends Component { | |
onMouseDown={ this.onPointerDown } | ||
onKeyDown={ this.onKeyDown } | ||
onFocus={ this.onFocus } | ||
className="editor-visual-editor__block-edit" | ||
className="editor-block-list__block-edit" | ||
tabIndex="0" | ||
aria-label={ blockLabel } | ||
> | ||
|
@@ -487,4 +512,4 @@ export default connect( | |
dispatch( editPost( { meta } ) ); | ||
}, | ||
} ) | ||
)( VisualEditorBlock ); | ||
)( BlockListBlock ); |
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.
Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?
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.
We could also use
context
and someHOCs
to specify the component to use for anchoring directly as well (rather than relying on DOM hierarchy and selectors).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 definitely agree with the desire here, and hope to discourage these hard-coded selectors from being introduced in the first place (this pull request serving as evidence of the maintenance pain they cause). But I'd rather address these separate from the intent of this pull request.
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.
Although given some of the immediate concerns of moving this as raised in #3546 (comment), maybe it is a prime time 😕