TypeAheadMenu and TableActionMenu rendered off screen fixed. #4301
Conversation
Shubhankerism
requested review from
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
Shubhankerism
changed the title
Shubhankerism
changed the title
|
Thank you, Shubhanker! This addresses #3834. |
| dropDownElement.style.opacity = '1'; | ||
| const dropDownElementRect = dropDownElement.getBoundingClientRect(); | ||
| let leftPosition = menuButtonRect.right + 5; |
There was a problem hiding this comment.
No magic numbers please, please put the 5 in a const above.
There was a problem hiding this comment.
Hi Ivo, thanks for the review :) I have replaced the number with a const now.
There was a problem hiding this comment.
Wow thank you! Been looking forward to seeing this menu fixed for many months now 🥹
@tylerjbainbridge mind having a pass to it, particularly the Typeahead area?
|
|
||
| const rootElementRect = rootElement.getBoundingClientRect(); | ||
|
|
||
| if (left + menuWidth > rootElementRect.right) { |
There was a problem hiding this comment.
I don't think it's incorrect to overlap the menu over other screen elements (even when this goes off the editor boundaries). However, when this goes beyond the viewport then that's a problem
There was a problem hiding this comment.
Hi @zurfyx, thanks for the feedback. :)
| @@ -600,11 +600,33 @@ function useMenuAnchorRef( | |||
| const positionMenu = useCallback(() => { | |||
| const rootElement = editor.getRootElement(); | |||
| const containerDiv = anchorElementRef.current; | |||
|
|
|||
| if (rootElement !== null && resolution !== null) { | |||
| const menuEle = containerDiv.getElementsByTagName('ul'); | |||
There was a problem hiding this comment.
I don't think hardcoding the ul is the right approach here.
That's how all of the playground typeaheads are configured, but we can't be sure that every outside user of the Plugin will implement it this way.
There was a problem hiding this comment.
Hi @tylerjbainbridge thanks for the review :) Now instead of using the ul element, I am referring to the TypeAheadMenu by ul element's parent div with the class name typeahead-popover component-picker-menu.
There was a problem hiding this comment.
Hi @Shubhankerism, unfortunately this still won't work because we can't count on those classes being applied to every typeahead. That's just how they're styled in our playground.
Let's use
const menuEle = containerDiv.firstChild;for now.
It's not perfect, but it'll be more robust than the other solutions. We can work on passing in a ref or something later on.
There was a problem hiding this comment.
Hi @tylerjbainbridge , thanks for the feedback. I have made the changes as you suggested.
|
|
||
| if (rootElement !== null && resolution !== null) { | ||
| const menuEle = containerDiv.getElementsByTagName('ul'); | ||
| if (rootElement !== null && resolution !== null && menuEle.length !== 0) { |
There was a problem hiding this comment.
If there is no ul in the menu's children it will skip over all of the positioning code here.
|
Can we please look into the 9 failing E2E tests before we can merge this? They seem to be consistently failing across platforms. |
Hi @zurfyx , I have pulled the recent changes from main and made the change that fixed the failing E2E tests. Can you please review it? Thanks :) |
In this PR, the issue related to the TypeAheadMenu and TableActionMenu rendered off screen is addressed.
Issue:
TypeAheadMenu rendered offscreen.
TableActionMenu rendered offscreen.
After changes: