Add the balloon to the bottom instead of the top when there isn't much space available. #1192
Conversation
…n't much space available.
westonruter
changed the title
|
How's that for timing. I just opened #1193 ! |
There was a problem hiding this comment.
There's a few coding style issues which are causing the Travis build to fail. I'd suggest either running npm test (or npm run lint) in your terminal before pushing code, or better yet, installing an editor integration which will display issues inline in your code.
editor/inserter/menu.js
Outdated
| @@ -225,7 +225,15 @@ class InserterMenu extends wp.element.Component { | |||
| render() { | |||
| const { position = 'top', instanceId } = this.props; | |||
| const visibleBlocksByCategory = this.getVisibleBlocksByCategory( wp.blocks.getBlockTypes() ); | |||
| const positionClasses = position.split( ' ' ).map( ( pos ) => `is-${ pos }` ); | |||
| const visualEditorHeight = document.querySelector( '.editor-visual-editor' ).clientHeight; | |||
There was a problem hiding this comment.
Ideally a component would not need to make any assumptions about the presence of any other elements on the page. In this case, it's both a little bit more reasonable because it's at least contained within editor-specific bundle, not a generic component, and because it's a difficult problem to solve (similar to in #1193 where I'd remarked about issue of knowing bounds of editor canvas).
editor/inserter/menu.js
Outdated
| @@ -225,7 +225,15 @@ class InserterMenu extends wp.element.Component { | |||
| render() { | |||
| const { position = 'top', instanceId } = this.props; | |||
| const visibleBlocksByCategory = this.getVisibleBlocksByCategory( wp.blocks.getBlockTypes() ); | |||
| const positionClasses = position.split( ' ' ).map( ( pos ) => `is-${ pos }` ); | |||
| const visualEditorHeight = document.querySelector( '.editor-visual-editor' ).clientHeight; | |||
| const minimumNeededHeight = 600; | |||
There was a problem hiding this comment.
An issue we might run into here is that the height of the inserter is dynamic depending on the viewport. Additionally, this could become difficult to maintain if we wanted to adjust the styling, since we'd similarly need to update component's JavaScript as well (and there's an assumption the unknown future maintainer knows to do this).
There was a problem hiding this comment.
@aduth then maybe if I looked up the height value using getComputedStyles on the .editor-inserter__content element it would make this solution more flexible?
There was a problem hiding this comment.
Yes, I think so. Or even clientHeight or getClientBoundingRect could work just as well.
It's not possible to do so here in the render though. Since the first time it would be called, there is not an actual DOM node yet (render only returns the React element). Instead, you'd need a combination of ref and componentDidMount (and more, per #1193 (comment)).
There was a problem hiding this comment.
I went on a different approach ... subtracting from the window height the height of the 3 elements that occupy vertical space around it (the admin bar, the editor header and the icon itself).
|
I fixed the linting errors. |
|
And now the height of the admin bar, the editor header and the button itself are computed on-render and subtracted from the available window height. Hope this helps! |
| const heightOccupied = | ||
| document.getElementById( 'wpadminbar' ).clientHeight + | ||
| document.querySelector( '.editor-header' ).clientHeight + | ||
| document.querySelector( '.editor-inserter__toggle' ).clientHeight; |
There was a problem hiding this comment.
If I click "Insert" in the editor bar, then click the inserter in post content, what node is returned by the query selector here? On the first render of the inserter in post content, it's actually the toggle from the header bar, which isn't correct.
Another one: If we decided to change the selector here to .editor-inserter__menu, what is the node on the first render? It wouldn't match anything (null) because the element hasn't yet been mounted yet. We're relying on the parent component already having mounted its toggle button, which is not a guarantee and is easy to overlook in refactoring.
I generally try to avoid component DOM querying because of these and other complexities. The render shouldn't assume that anything is present in the DOM, nor that it's the only instance of the component. To my previous comment, if DOM access is needed, it should be done via ref.
More accurately, always present at this current point in time. Could lead to some maintenance pain if this were ever to change. |
|
Closed in favor of the approach in #1193. |
Fixes #1004
I increased the minimum height of the overflow wrapper while avoiding adding an unnecessary vertical scrollbar (hence the calc).
Before showing the balloon, the height of the visual editor is checked and switches the position from top to bottom.