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.

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

Detect Inserter bounds collision by generic Popover component #1193

Merged
merged 3 commits into from
Jun 15, 2017

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 15, 2017

Closes #1098
Closes #1004

This pull request seeks to resolve an issue where the inserter is clipped by the editor bar when opened on a new post. A Popover component has been extracted from InserterMenu to provide a generic solution to tooltip-like behavior, including boundary collision detection. When the component mounts or its desired position changes, if it exceeds the viewport boundaries, its direction will be flipped.

Implementation notes:

Similar to as observed in #839, it is difficult to know from the context of the Popover component the bounds of the editor canvas. In this implementation I merely check whether it exceeds the bounds of the entire page (top = 0, left = 0). This could benefit from extensibility allowing the editor layout to override the default bounds (again, since the component is generic, we should not bind it to the editor use-case).

Testing instructions:

Verify that there are no regressions in inserter behavior.
Verify that the editor bar inserter continues to open downward.
Verify that in a post with sufficient content, the inserter opens upward.
Verify that if the inserter were to overlap with the editor bar, it opens downward instead.

@mehigh
Copy link
Contributor

mehigh commented Jun 15, 2017

@aduth
As I looked into your PR and the comments inside it
"Verify that if the inserter were to overlap with the editor bar, it opens downward instead"
This is something my simpler implementation does not cover right now, as I didn't experience this specific thing myself.

Nonetheless increasing the min-height of the overflow element is needed in your implementation to improve it even further.

forcedPositions[ dir ] || ( ! result && includes( positions, dir ) )
? dir
: result
), null ) || defaultDirection;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we store forced positions like this { forceYAxis: 'top', forceXAxis: 'left' } we could simplify the logic here and avoid some loops.

@youknowriad
Copy link
Contributor

I really like this generic component, great improvement. I think we should make the arrow optional and use this component for:

  • Block Toolbars
  • Dropdown menus (block switcher, post schedule, post visibility...)

Granted this could be done separately.

Some notes:

  • Should we recompute if the window is resized?
  • Should we recompute if the popover size/position changes?

@aduth
Copy link
Member Author

aduth commented Jun 15, 2017

Nonetheless increasing the min-height of the overflow element is needed in your implementation to improve it even further.

Can you clarify the need here? I did find that originally I'd needed to change an overflow: auto; to overflow: visible; in the layout styling, but that this was no longer necessary when I rebased because the overflow was removed altogether in 7658b48.

@aduth
Copy link
Member Author

aduth commented Jun 15, 2017

  • Should we recompute if the window is resized?
  • Should we recompute if the popover size/position changes?

Yes to all of the above. It should already recompute if position changes. "Size" is hard to measure; maybe children changes? But those changes are difficult (impossible?) to detect (related).

@aduth
Copy link
Member Author

aduth commented Jun 15, 2017

I think if we store forced positions like this { forceYAxis: 'top', forceXAxis: 'left' } we could simplify the logic here and avoid some loops.

This simplified things a fair bit. Was able to drop both deep equality check and the reduce in 8ac1fd2.

@mehigh
Copy link
Contributor

mehigh commented Jun 15, 2017

@aduth you can take a look over what I committed last on #1192 and see whether it comes in handy or not for wrapping up the solution for this.

Thanks!

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍 It'd be great to have this merged soon, this is an annoying bug :)

@aduth aduth added General Interface Parts of the UI which don't fall neatly under other labels. [Feature] Inserter The main way to insert blocks using the + button in the editing interface labels Jun 15, 2017
@aduth aduth merged commit 5069c7d into master Jun 15, 2017
@aduth aduth deleted the update/1098-inserter-collision branch June 15, 2017 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Inserter The main way to insert blocks using the + button in the editing interface General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

On blank page open inserter downward Inserter popover gets cutoff
3 participants