Skip to content
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.

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

render isn't recognized as one of the three MenuItem spec options #13

Closed
adamdonahue opened this issue Jan 16, 2018 · 4 comments
Closed

Comments

@adamdonahue
Copy link
Contributor

adamdonahue commented Jan 16, 2018

image

The docs above indicate that defining either a label, an icon, or a render method should be sufficient for a MenuItem. And yet after upgrading from 0.21 to 0.24 all our renderable menu items are now resulting in an error:

image

I didn't see anything in the changelog indicating this new behavior, but I'm surprised nobody has reported it before. Is this a regression?

@adamdonahue adamdonahue changed the title render isn' render isn't recognized as one of the three MenuItem spec options Jan 16, 2018
@adamdonahue
Copy link
Contributor Author

Also note that the MenuElement interface definition of render isn't consistent with the MenuItem definition. So this is confusing all around:

  • Is render supported in MenuItem now, at all?
  • If it is, should it return a dom element alone (as the docs still indicate)?
  • Or should it return an object with a dom property and an update one. In that case, is there a default, and reasonable, update function?

I was able to "fix" this (assuming it's not just a mistake on my part) locally by check for render in addition to label and icon, and having render of the item return a single DOM element, but this is just a quick-and-dirty fix.

@marijnh
Copy link
Member

marijnh commented Jan 16, 2018

Why are you using 0.24 when there's a 1.0.1? That one should have docs that are consistent with its code. And yeah, using dom and update is how the menu works now.

@marijnh marijnh closed this as completed Jan 16, 2018
@adamdonahue
Copy link
Contributor Author

The code looked the same, but I'll give it a shot.

@adamdonahue
Copy link
Contributor Author

Nope, that didn't work (despite following the documentation). I hadn't noticed the experimental nature of this repository, though, so we'll just clone and work on our own customized implementation. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants