Fix minor bugs in submenu#1463
Conversation
|
@ang-zeyu Sorry missed out on a few bugs in the previous PR 😅. Would appreciate if you are able to look through and review it :) |
| </include> | ||
|
|
||
| **You can also use create multi-level submenu by nesting Dropdowns.** | ||
| **You can also create multi-level submenu by nesting Dropdowns.** |
There was a problem hiding this comment.
| **You can also create multi-level submenu by nesting Dropdowns.** | |
| **You can also create multi-level submenus by nesting Dropdowns.** |
| e.preventDefault(); | ||
| if (window.innerWidth > 767) { | ||
| if (this.disabledBool) { return false; } | ||
| if (this.show || this.disabledBool) { return false; } |
There was a problem hiding this comment.
Currently, submenu will call the method showSubmenu again even if it is opened. For some edge cases, this may cause the submenu to re-position and cause it to overflow. This can be fixed by preventing submenu from calling showSubmenu again if it is already opened.
Hmm how is this reproduced? Can't seem to find an edge case
If anything, shouldn't repositioning it 'fix' the position? (e.g. open submenu -> not overflown -> scroll down such that it is -> overflown -> hover again -> not overflown)
There was a problem hiding this comment.
The isRightAlign method checks whether the current position of the submenu is overflowing on the right side, if it is, then it switches to the left side. However, if it is already on the left side, then it is not overflowing when the check happens hence moving the submenu back to the right side.
If anything, shouldn't repositioning it 'fix' the position? (e.g. open submenu -> not overflown -> scroll down such that it is -> overflown -> hover again -> not overflown)
I guess one workaround to solve both of these is to not call the isRightAlign method if the submenu is already opened.
There was a problem hiding this comment.
After looking around I think a better way to do it is to keep it on the left if it is opened and already on the left. Else if it is on the right, will check for overflow and flip to left if necessary.
There was a problem hiding this comment.
Thanks for the gif!
After looking around I think a better way to do it is to keep it on the left if it is opened and already on the left. Else if it is on the right, will check for overflow and flip to left if necessary.
How does the fix look like? If its too complex, could leave it as is. Repositioning on-hover should really never happen anyway =P (and we don't have reposition on-resize, etc.)
There was a problem hiding this comment.
The fix I had in mind is to insert this additional rule, on line 64, in the showSubmenu method to check if the submenu is already on the left side. If it is, then it will continue to be on the left side.
if (!this.dropleft && positionSubmenu.isRightAlign(ul)) {
this.alignMenuRight();
} else {
this.alignMenuLeft();
}
I'm also fine with leaving it as it is because this scenario is rare and the fix does not fully solve the issue (just preventing a re-positioning if it is on the left side only).
There was a problem hiding this comment.
I'm also fine with leaving it as it is because this scenario is rare and the fix does not fully solve the issue (just preventing a re-positioning if it is on the left side only).
ok, let's leave it as is then. once all of #980 is done we could explore a more robust repositioning implementation (e.g. with reposition on resize).
There was a problem hiding this comment.
Alright sounds good 👍
Have also updated this PR :)
There was a problem hiding this comment.
meant to leave the this.show in the current pr (prevent repositioning) =X
There was a problem hiding this comment.
Oops 😬 Added back :)
2 participants
What is the purpose of this pull request?
Overview of changes:
Fixes minor bugs present in the
submenufor the PR #1455.submenusection underdropdown.submenuwill call the methodshowSubmenuagain even if it is opened. For some edge cases, this may cause thesubmenuto re-position and cause it to overflow. This can be fixed by preventingsubmenufrom callingshowSubmenuagain if it is already opened.Anything you'd like to highlight / discuss:
NIL
Testing instructions:
npm run testProposed commit message: (wrap lines at 72 characters)
Fix minor bugs in submenu
Checklist: ☑️