Skip to content
This repository has been archived by the owner on Nov 30, 2020. It is now read-only.

Menu selected event is fired twice #480

Closed
ErikMinekus opened this issue Nov 14, 2019 · 5 comments · Fixed by #534
Closed

Menu selected event is fired twice #480

ErikMinekus opened this issue Nov 14, 2019 · 5 comments · Fixed by #534
Assignees

Comments

@ErikMinekus
Copy link
Contributor

Describe the bug
When clicking a menu item, the selected event is fired twice. This started happening after updating from version 1.1.0 to 1.2.0.

To Reproduce

  1. Go to https://codepen.io/ErikMinekus/pen/KKKGXOp
  2. Open the developer console
  3. Click on 'Open menu'
  4. Click on an entry
  5. See that console.log is fired twice

Expected behavior
The selected event should only be fired once, as it does when using the MDC Menu component (v3.2.0) directly. See https://codepen.io/ErikMinekus/pen/wvvYPmK.

Desktop

  • OS: macOS Mojave 10.14.6
  • Browser: Chrome 78
  • Version 1.2.0
@ErikMinekus
Copy link
Contributor Author

ErikMinekus commented Nov 18, 2019

The issue is caused by the List component, see c24f7c8. It used to have these lines in mounted():

if (this.$parent.$options._componentTag !== 'm-menu') {
  this.mdcList = MDCList.attachTo(this.$el)

This line was deleted, so now you have to set the js prop to false to ensure it doesn't instantiate the MDCList component. Except this causes a JavaScript error here:

onAction (e) {
  this.$emit('change', this.mdcList.selectedIndex)

since this.mdcList is not instantiated.

@matsp
Copy link
Owner

matsp commented Nov 18, 2019

@ErikMinekus Thanks for finding the issue! Feel free to submit a PR 👍

@tychenjiajun
Copy link
Contributor

@ErikMinekus So sorry that I made a mistake in previous versions. JavaScript component is optional in some components like List in mdc-web. That's why js is introduced. And some components like Menu, Drawer and Tab Bar can instantiate their children component, which may cause duplicate instantiation. I've fixed this issue in Tab Bar in #443 and discuss some solutions in #113. But there's still lots of work to do on the other components including Menus. Excuse for any mistakes in my explanations and feel free to help or submit a PR.

@ErikMinekus
Copy link
Contributor Author

@tychenjiajun I'm relatively new to Vue and Material components, but I think for Menu it would be best to just hardcode <ul class="mdc-list"> in the Menu component, like in the enhanced Select. This would be a breaking change, but at least people can't forget to set js to false.

I'm not familiar with how Drawer and TabBar can instantiate child components, so I can't comment on that. But it should be documented when you have to set js to false.

@tychenjiajun
Copy link
Contributor

@ErikMinekus In the enhanced Select, List is hardcoded to make enhanced reactive. So I think it's better to keep it hardcoded in mdc-web version 3.x.x. Since 4.0.0, the basic Select is deprecated and only enhanced Select is kept. So our enhanced will be deprecated too. I thinks it's ok to introduce more breaking change when updating to mdc-web 4.0.0 so Select will be changed to the same style as what Menu is now.

Whatever, thank you for this issue and PR!

@tychenjiajun tychenjiajun self-assigned this Jan 29, 2020
tychenjiajun added a commit to tychenjiajun/material-components-vue that referenced this issue Jan 30, 2020
tychenjiajun added a commit that referenced this issue Jan 31, 2020
* fix(menu): use provide/inject in instantiation

fixes #480

* fix(menu): update demo
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants