Improve fake menu support #23
Conversation
…ke-menu # Conflicts: # src/components/ebay-menu/marko-tag.json
|
Yes, certainly minimal impact in terms of code logic and complexity. I've been playing with it and it appears to meet all the requirements for an ARIA menu and fake menu. As we don't really have compelling evidence one way or the other whether to split this or not, I'm inclined to say we merge this PR, and go with this for first release. We may or may not run into some issues when we get to the concept of grouping. If at some point down the line we feel like we need to split the component, we can always do that and slowly phase out and deprecate the fake features of this component. One question: how do the events work with anchors and buttons? One observation: I feel the need to specify Also, I don't think the skin issue is a blocker, so I removed that label. |
| classes.push('fake-menu__item'); | ||
| if (href) { |
There was a problem hiding this comment.
Should this be two types, button and link? Otherwise the developer has to think hard about how to create the right kind of menu item they want.
There was a problem hiding this comment.
I'm ok with allowing link, it's just that it doesn't seem like it would actually do anything. If you have a link menu-item, you have to send an href no matter what (right?). If we allow type=link, would that mean that you have to send both, or else we don't treat it like a link? Or do we still treat it as link even if you only specify href?
There was a problem hiding this comment.
To me, it doesn't make sense to have type for only one thing. If we have a type, we should allow the developer to specify it for all types. Otherwise, their logic gets really cumbersome.
There was a problem hiding this comment.
I agree with Sean. I even think we should allow type="menuitem" on the root. Even though menu item is the default (not checkbox or radio). This is like HTML, the default can be specified or not.
There was a problem hiding this comment.
This can be taken on as a separate PR, if necessary, to achieve consistency across all components.
|
@ianmcburnie The events function identically to the regular (not radio/checkbox) case, and will emit |
|
I think these events are sufficient to begin with. We can always update based on feedback after 1st release. |
Description
fakeboolean in favor of usingtype(alongside "radio" and "checkbox")menuitemrole for fake casetype="button"formenu-itemContext
The actual changes here are pretty minimal, and there isn't much of a code split for fake vs regular right now, so I'd be inclined to keep it in the
menucomponent until the complexity warrants simplification. @ianmcburnie Do you anticipate other changes to accommodate this, that I might be missing?References
Note that there is a minor issue with Skin for this case that has not been resolved yet:
eBay/skin#83