Skip to content

Fix minor bugs in submenu #1463

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.

Sign up for GitHub

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

Merged
ang-zeyu merged 3 commits into from
Jan 30, 2021
Merged

Fix minor bugs in submenu #1463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c9a66ff
Fix minor bugs for submenu
jonahtanjz Jan 28, 2021
6a11aa2
Update after review
jonahtanjz Jan 30, 2021
61eab5f
Prevent repositioning
jonahtanjz Jan 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/userGuide/syntax/dropdowns.mbdf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
</variable>
</include>

**You can also use create multi-level submenu by nesting Dropdowns.**
**You can also create multi-level submenus by nesting Dropdowns.**

<include src="codeAndOutput.md" boilerplate >
<variable name="highlightStyle">html</variable>
Expand Down
2 changes: 1 addition & 1 deletion packages/vue-components/src/Submenu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export default {
$el.findChildren('a,button').on('mouseover', (e) => {
e.preventDefault();
if (window.innerWidth > 767) {
if (this.disabledBool) { return false; }
if (this.show || this.disabledBool) { return false; }

@ang-zeyu ang-zeyu Jan 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

@jonahtanjz jonahtanjz Jan 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ezgif-7-63eaab259163

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.

@jonahtanjz jonahtanjz Jan 29, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@ang-zeyu ang-zeyu Jan 29, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.)

@jonahtanjz jonahtanjz Jan 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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).

@ang-zeyu ang-zeyu Jan 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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).

@jonahtanjz jonahtanjz Jan 30, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Alright sounds good 👍

Have also updated this PR :)

@ang-zeyu ang-zeyu Jan 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

meant to leave the this.show in the current pr (prevent repositioning) =X

@jonahtanjz jonahtanjz Jan 30, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops 😬 Added back :)

e.currentTarget.click();
this.showSubmenu();
}
Expand Down