Support sub-menu for dropdown by jonahtanjz · Pull Request #1455 · MarkBind/markbind

jonahtanjz · 2021-01-21T03:48:47Z

What is the purpose of this pull request?

Overview of changes:
Resolves #212.

Sub-menus can be added by nesting dropdowns so that users do not have to remember a new syntax. A new component <submenu> is created to handle the logic. <dropdown> component is edited such that if a <dropdown> is nested, it will be converted to a <submenu>. Sub-menus can also contain sub-menus using the same syntax for multiple levels. Currently, sub-menus will appear to the right of a dropdown on desktop version and becomes an accordion on mobile.

Anything you'd like to highlight / discuss:
Implementation.

Testing instructions:
Sub-menus can be added by nesting <dropdown>.

For example:

<dropdown>
    <li>...</li> // normal dropdown item
    <li>...</li>
    <dropdown> // will be converted to submenu
        <li>...</li> // submenu item
        <li>...</li>
        <dropdown> // 2nd-level submenu
            <li>...</li> // 2nd-level submenu item
            <li>...</li>
        </dropdown>
    </dropdown>
</dropdown>

A dropdown with 2 levels of submenu will be created.

Proposed commit message: (wrap lines at 72 characters)
Add sub-menu support for dropdown

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No blatantly unrelated changes
Pinged someone for a review!

jonahtanjz · 2021-01-21T03:51:21Z

@ang-zeyu this is ready for review and do let me know on how this can be improved. If the implementation is fine, will proceed to add in tests and documentation as well.

ang-zeyu

Just a review on the overall approach / issues:

I was thinking of using the https://v3.vuejs.org/guide/component-provide-inject.html#working-with-reactivity.

e.g.

you could do

<dropdown>
  <div class="some-wrapper">
    <dropdown></dropdown> // would still work when nested
  </div>
</dropdown>

the approach here is more consistent with vue-strap's, but as you might have observed its a rather low level approach that ties itself heavily to the dom. (note vue-strap didn't have this api at the time)

But I would be ok with accepting this as quite a bit of work has been done already. If you're up for it, let's change this over to provide/inject (could even make a separate PR to shift the original implementation to it first if it makes things easier)

Positioning of submenus needs to be dynamic (we can assume right position first then flip it over to the left otherwise). (note the submenu may overflow over the screen currently) You could explore the popperjs library to do this (bootstrap-vue uses this internally for its popovers,dropdowns...)
Really small nit: Make the submenu show on-hover as well? 🤔

ang-zeyu · 2021-01-21T06:26:38Z

One more note since you're tackling #980 as well: you may want to finish shifting the mobile navbar design to it first, as dropdowns contained within navbars look / behave quite differently - less work to do here

jonahtanjz · 2021-01-25T06:34:21Z

@ang-zeyu I've made some changes accordingly.

I was thinking of using the https://v3.vuejs.org/guide/component-provide-inject.html#working-with-reactivity.

I didn't use provide/inject in the end as when I was trying to implement this, I managed to remove all the unnecessary communication between the dropdown and submenu components. Not sure where else to use it as the rest of the search of the virtual DOM seems necessary, for example listening for clicks on li>a as these are user defined.

Positioning of submenus needs to be dynamic (we can assume right position first then flip it over to the left otherwise). (note the submenu may overflow over the screen currently) You could explore the popperjs library to do this (bootstrap-vue uses this internally for its popovers,dropdowns...)

I tried out popperjs but the auto positioning didn't work very well/a little buggy. I switched to using a modified bootstrap javascript which similarly solves this issue. Hopefully that's fine ;)

I've also added tests and updated the docs.

ang-zeyu

I didn't use provide/inject in the end as when I was trying to implement this, I managed to remove all the unnecessary communication between the dropdown and submenu components.

Not so much for removing unnecessary communication but just allowing parent / child communication across different compilation scopes.

Nice work isolating the implementation as much as possible - only have one not-so-beneficial suggestion where this can be used now. (see comment below)

You could take a look at the following for examples how it could be used more beneficially:

bv components
our own <quiz> / <question> / <q-option> components

Not sure where else to use it as the rest of the search of the virtual DOM seems necessary, for example listening for clicks on li>a as these are user defined.

yes this is still necessary unfortunately, due to our use of standard dom elements.

I tried out popperjs but the auto positioning didn't work very well/a little buggy. I switched to using a modified bootstrap javascript which similarly solves this issue. Hopefully that's fine ;)

Seems ok too 👍

Some strange behaviours:

(main / submenu closing on hover)

(this is on mobile view)

Some suggestions:

ang-zeyu · 2021-01-25T09:03:04Z

    </slot>
  </li>
+  <submenu v-else-if="isSubmenu" ref="submenu">
+    <slot slot="button" name=button></slot>


could do

<template v-for="(node, name) in $slots" :slot="name"> <slot :name="name"></slot> </template>

Thanks! Updated :)

ang-zeyu · 2021-01-25T09:12:57Z

      return toBoolean(this.disabled);
    },
    isLi () { return this.$parent._navbar || this.$parent.menu || this.$parent._tabset },
+    isSubmenu() { return this.$parent._dropdown || this.$parent._submenu },


simple example where provide / inject could be used to remove the $parent dependency, but not so beneficial:

provide: { hasParentDropdown: true } inject: ['hasParentDropdown'] isSubmenu() { return this.hasParentDropdown}

Thanks for the clarification. Have added this in :)

ang-zeyu · 2021-01-25T10:02:16Z

+      type: Boolean,
+      default: false,
+    },
+    'class': null,


Thanks! Updated :)

ang-zeyu · 2021-01-25T10:03:13Z

+  },
+  computed: {
+    classes() {
+      return [


let's place this directly in the template only for consistency with other components'

Alright. Updated :)

ang-zeyu · 2021-01-25T10:08:07Z

+<include src="codeAndOutput.md" boilerplate >
+<variable name="highlightStyle">html</variable>
+<variable name="code">
+<!-- Can simply nest the dropdown syntax -->


Suggested change

ang-zeyu · 2021-01-25T10:11:27Z

+    },
+    addClassIfSubmenu(a, li) {
+      let el = a.parentElement;
+      while (el != li) {


Thanks! Updated :)

ang-zeyu · 2021-01-25T10:20:40Z

+  },
+  methods: {
+    hideSubmenu() {
+      this.show = false;


let's use data() for properties that could change

ang-zeyu · 2021-01-25T10:26:10Z

+}
+
+@media (max-width: 767px) {
+  .dropdown-submenu > ul {


hmm can this be moved to the same media query below?

Thanks! Updated :)

jonahtanjz · 2021-01-25T14:56:41Z

@ang-zeyu Thanks for the suggestions and clarification 😄. I've updated it accordingly.

Some strange behaviours:
(main / submenu closing on hover)

For the main menu, it is closing because when the inner children of the submenu toggle is clicked/hovered, it triggers a blur event, which closes the main dropdown menu. I've fixed this by preventing this specific event from bubbling up.

As for the submenu closing on hover, it is because when the user hovers over the submenu toggle, it will mirror a click. That is the reason why the second hover will close the submenu. One workaround is to only allow submenus to be opened by hovering over and not by clicking. This is the current implementation for now.

ang-zeyu

Couple more nits:

ang-zeyu · 2021-01-27T03:31:08Z

      this.show = true;
      $(this.$refs.dropdown).findChildren('ul').each(ul => ul.classList.toggle('show', true));
-    },
+    }


let's correct these in another PR; we could enable linting for dropdown.vue as well. (the unlinted ones are files that haven't changed much from vue-strap)

Alright sure will open another PR for this 👍

ang-zeyu · 2021-01-27T03:33:01Z

+    disabledBool() {
+      return toBoolean(this.disabled);
+    },
+    showBool() {


this (could replace with just show) and slots() dosen't seem used

Thanks! Replaced all showBool with show

ang-zeyu · 2021-01-27T03:36:16Z

+</script>
+
+<style scoped>
+.dropdown-submenu {


let's indent these to fit the stylelint rules too (they've never been validated in our scripts for .vue files but should be)

jonahtanjz · 2021-01-27T04:11:23Z

@ang-zeyu PR is updated accordingly :)

ang-zeyu

Lgtm 👍

jonahtanjz added 3 commits January 20, 2021 17:48

Add submenu component

57a8329

Add submenu functions and styling

d0942f5

Add highlighting for submenu containing current page

1fd3231

ang-zeyu reviewed Jan 21, 2021

View reviewed changes

jonahtanjz added 2 commits January 24, 2021 22:07

Prevent overflow of submenu

be946d0

Open submenu on hover

7164878

Revert tests

a26f606

jonahtanjz force-pushed the 212-dropdown-submenu branch from 716e2f9 to a26f606 Compare January 25, 2021 07:03

ang-zeyu reviewed Jan 25, 2021

View reviewed changes

jonahtanjz added 2 commits January 25, 2021 22:41

Update after review

969a96f

Update tests

e6fa314

Add method in submenu

4bc69a2

ang-zeyu reviewed Jan 27, 2021

View reviewed changes

Update after review

bcb7af3

ang-zeyu approved these changes Jan 27, 2021

View reviewed changes

ang-zeyu added this to the v3.0 milestone Jan 27, 2021

ang-zeyu merged commit 3c3f295 into MarkBind:master Jan 27, 2021

This was referenced Jan 28, 2021

Enable linting for dropdown.vue #1462

Merged

Fix minor bugs in submenu #1463

Merged

	<!-- Can simply nest the dropdown syntax -->
	<!-- Nest the dropdown syntax to create dropdown submenus -->

Uh oh!

Conversation

jonahtanjz commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahtanjz commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

ang-zeyu commented Jan 21, 2021

Uh oh!

jonahtanjz commented Jan 25, 2021

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahtanjz commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahtanjz Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahtanjz commented Jan 27, 2021

Uh oh!

ang-zeyu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jonahtanjz commented Jan 21, 2021 •

edited

Loading

jonahtanjz commented Jan 21, 2021 •

edited

Loading

jonahtanjz commented Jan 25, 2021 •

edited

Loading

jonahtanjz Jan 27, 2021 •

edited

Loading