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

Menu widget #52

Closed
rishson opened this issue Mar 6, 2017 · 37 comments
Closed

Menu widget #52

rishson opened this issue Mar 6, 2017 · 37 comments

Comments

@rishson
Copy link
Contributor

rishson commented Mar 6, 2017

Create a menu widget.

Initial thoughts: menu as in 'full width page header that contains links to other pages | dropdowns | logos | searchbox etc.

Spec:

  • list of callback funcs that can be passed in props
    some kind of 'onHide' or 'onPageScroll' that indicates when the menu would be scrolled off screen (so controlling component can reduce hight etc)

  • mouse/keyboard interactions (if any)
    as per individual controls passed to menu

  • mandatory/valid/empty/wait states (if any)
    as per individual controls passed to menu

  • value-add of the widget (e.g. why use rather than just use VDom directly)
    ability to easily style to make sticky, align to top, bottom, left, right..
    responsive on mobile
    ability to vertically align items of different heights (for a horizontal menu)

  • is the widget controlled/uncontrolled
    I am imagining that you would pass the menu child widgets, therefore it is controlled.

  • list of any icons needed
    none

  • is the widget responsive
    yes, on mobile, this widget should shrink to a hamburger menu

@mwistrand mwistrand self-assigned this Mar 6, 2017
@eheasley eheasley added this to the 2017.03 milestone Mar 6, 2017
@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

Here's what I was thinking:

A NavBar would go across the top of a page and can contain a logo or header text. It can also contain a list of MenuItems, Menus that contains MenuItems, or both (think Bootstrap NavBars with links and dropdowns to other links). The parent NavBar should collapse its child MenuItems and Menus to use a SlidePanel to show them when screen width goes below a designated breakpoint.

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

Do we want to build the static/fixed position into this, though? I think that might be good for a separate container widget, since it's pretty common to want all sorts of random things in a scrolling header (or footer).

@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

I think a MenuItem should be able to be whatever you want (such as a search input). I also think the NavBar should be fixed (and thus collapse its children into a SlidePane) based on a property. If that property is false, the menu isn't fixed and doesn't collapse.

@mwistrand
Copy link
Contributor

Rather than limiting a NavBar to just a logo/header text and menu items, it would be better to allow any number of children as required, with any menu items being contained in a separate Menu widget.

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

@bitpshr I'm not sure the fixed positioning should be tied to collapsing into a SlidePane. What if someone wants a desktop-width uncollapsed fixed nav bar? I like allowing MenuItems to be anything, that solves the flexible scrolling header issue.

@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

Maybe we could use separate properties to determine menu collapsing and fixed positioning?

@mwistrand: I'm not sure I follow. When I say "MenuItem", I just mean a container to put a link or input or any other component in, that handles styling in an inline-block manner to stay consistent. I also don't think any MenuItem should be within a Menu. Think Bootstrap. Some menu items are within a dropdown menu, some aren't.

@mwistrand
Copy link
Contributor

Depending on the number of items within the nav bar, perhaps a breakpoint property could be used to determine the point at which the menu is expanded from the SlidePane?

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

Sounds good! So, let me know if I have this right... we have a NavBar widget that handles fixed/static positioning, resize events and collapsing, and showing/hiding its children, and a Menu widget that's more like a list, and a MenuItem that sounds like it could be a property of NavBar or Menu that can take a widget or DNode

@mwistrand
Copy link
Contributor

@bitpshr When I talk about "menu items being contained in a separate Menu widget", I'm assuming Menu is generic enough to be used in multiple ways (e.g., as a horizontal list or a drop down menu). In this case, if a NavBar is really just an application header, then grouping related menu items beneath Menus allows the nav bar to be more flexible:

w(NavBar, [
    v('img', { src: 'path/to/logo.png' }),
    w(Menu, [ /* "login" and "join" links */ ]),
    w(Menu, [ /* app navigation */ ])
]);

@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

If "login" and "join" are individual links, it's confusing to me to consider them as part of the same Menu. Why? How is that more flexible?

This is clearer to me:

w(NavBar, [
    v('img', { src: 'path/to/logo.png' }),
    w(MenuItem, [ /* "login" link */ ]),
    w(MenuItem, [ /* "join" link */ ]),
    w(Menu, [ /* app navigation */ ])
]);

@mwistrand
Copy link
Contributor

@bitpshr Disregard. I think I just misunderstood your original statement about the possible contents of NavBar.

@rishson
Copy link
Contributor Author

rishson commented Mar 6, 2017

Just to clarify, I think that a navbar should be able to take any kind of child - I should have been more explicit that I was just giving examples at the start.

@rishson
Copy link
Contributor Author

rishson commented Mar 6, 2017

w(NavBar, [
    v('img', { src: 'path/to/logo.png' }),
    v('a', [ /* "login" link */ ]),
    v('a', [ /* "join" link */ ]),
    w(Menu, [ /* app navigation */ ])
]);

?

One thing we would have to do, would be to decide how to vertically align items (for a horizontal nav). Thats a piece of cake with flexbox right?

@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

MenuItem in my mind was purely a convenience because it could handle alignment, but maybe that's not necessary. @rishson's example above seems fine. And yes, that's easy enough with flexbox.

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

I guess the one other question is about accessibility. A nav bar should be role="navigation", and links within it should probably have role="menuitem", which would be easy with a MenuItem widget. Not sure if it's worth it, though, since it's still obvious they're links within a menu. If consumers leave it out, it's not too poor for usability.

@mwistrand's going to have a fun time figuring out how to handle focus, though. Tabbing should probably move you into the menu or out of the menu, with arrow keys handling navigation through it. Allowing any widgets into it is going to be a pile of fun I hadn't thought about until just now

@rishson
Copy link
Contributor Author

rishson commented Mar 6, 2017

Should we look to rename this issue navbar and have menu as something closer to a top-level item that reveals menu options when selected?

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

Putting this here for a11y reference: https://www.w3.org/TR/wai-aria-practices-1.1/#menu

And this as an example: https://www.w3.org/TR/wai-aria-practices-1.1/examples/menubar/menubar-1/menubar-1.html

@bitpshr
Copy link
Member

bitpshr commented Mar 6, 2017

@rishson: Sure, though I think a Menu will be tightly tied to this. @smhigley agreed, but I think a few of our components will have tough times with focus. I've been putting that off in hopes we get a focus manager at some point.

@smhigley
Copy link
Contributor

smhigley commented Mar 6, 2017

@bitpshr we should probably start noting down all the places a focus manager could be useful. I was just thinking of it as a way to trap focus within a certain element (like a dialog or flyout menu), but I imagine it would be useful for something like this too. I'm just not entirely sure how that would work or what it would do.

@eheasley eheasley added the beta2 label Mar 7, 2017
@mwistrand
Copy link
Contributor

Before I get any deeper into this, it would be wise to get some feedback regarding the Menu widget.

A menu is basically a list of children with predefined layout properties. By default, a Menu can:

  • be displayed as a horizontal or vertical list (default is vertical)
  • be always displayed, or toggled in and out of view by clicking or mousing over a label node
  • be a drop down, or an accordion-style menu (default is accordion-style)
  • have any string or DNode children
  • be nested within another menu

The properties used to control the menu would resemble the following:

interface MenuProperties {
  /**
   * If `true` then all menu items will be disabled. If a `label` is specified,
   * then the menu will not be displayed when disabled.
   */
  disabled?: boolean;
  
  /**
   * Indicates whether the menu is rendered as a drop down or accordion-style menu
   * (the default). Only relevant if a `label` is specified.
   */
  dropDown?: boolean;
  
  /**
   * Defaults to `false` if no `label` is specified, but defaults to `true` if a
   * `label` widget is provided.
   */
  hidden?: boolean;
  
  /**
   * Determines whether the menu is rendered as a horizontal list or a vertical list.
   * Defaults to `false`.
   */
  horizontal?: boolean;
  
  /**
   * An optional widget used to toggle the menu into view.
   */
  label?: string | DNode;
  
  /**
   * An optional function to control how the menu is hidden.
   */
  onHide?: (menu) => void;
  
  /**
   * An optional function to control how the menu transitions from hidden to displayed.
   */
  onShow?: (menu) => void;
  
  /**
   * Determines whether the menu is displayed when `label` is clicked, or via
   * mouse hover. Defaults to `true`.
   */
  triggerOnClick?: boolean;
}

Question: does the optional label widget work, or would it be better in the long run to separate this functionality out into something like a MenuLabel widget?

@tomdye
Copy link
Member

tomdye commented Mar 8, 2017

As per the conversation around MenuItem or any kind of child, we may at least need a class or way of indicating that the child should be considered a single MenuItem for layout purposes. Without this we may end up with nasty .navBar * selectors for spacing / alignment.
I think we should split this out into NavBar and MenuItem perhaps and ensure that we focus on the value adds such as making the nav bar sticky and collapsing it into a sidepane, otherwise the user may as well just use header tags and write their own styling.

@smhigley
Copy link
Contributor

smhigley commented Mar 8, 2017

This might be nitpicky, but could we only use label to refer to actual labels? (Where an actual label is either a <label> tag or something referred to by aria-labelledby)

@mwistrand mwistrand mentioned this issue Mar 21, 2017
3 tasks
@dylans dylans removed this from the 2017.03 milestone Apr 2, 2017
@smhigley
Copy link
Contributor

Current status of Menu:

We now have the ability to type children, so it is more feasible to just create Menu with type-enforced MenuItem children. TabController currently does this, and overrides certain child properties with:

.map((tab, i) => {
   assign(tab.properties, {...});
   return tab;
})

It doesn't really solve the problem of creating public properties on MenuItem that are effectively private, since they will always be reset in Menu. However, I'm comfortable with continuing this pattern for now so we can at least create a working widget, then refining it later.

@kitsonk kitsonk modified the milestones: 2017.10, beta.4 Oct 30, 2017
@bitpshr bitpshr removed their assignment Nov 1, 2017
@morrinene morrinene removed the future label Nov 20, 2017
@kitsonk
Copy link
Member

kitsonk commented Nov 29, 2017

Fixed by #104

@kitsonk kitsonk closed this as completed Nov 29, 2017
@bitpshr
Copy link
Member

bitpshr commented Nov 30, 2017

#104 was closed and was never merged. I think we need to leave this issue open to track the new Menu implementation.

@bitpshr bitpshr reopened this Nov 30, 2017
@kitsonk kitsonk modified the milestones: beta.4, beta.5 Dec 1, 2017
@smhigley
Copy link
Contributor

smhigley commented Dec 6, 2017

Some comments on the potential usage of Listbox within the Menu widget:

On the surface, it seems like a good fit, since in both you are presented with a list of things to interact with, and choose one (or more). Under that though, Listbox is fundamentally a selection widget, and Menu is a list of actionable items with a potentially more complex structure.

First, the semantic things that would need to be changed from Listbox:

  • roles of menu and menuitem instead of listbox and option
  • Menu items themselves should be links, not divs
  • Should probably not have aria-selected on menu items
  • I don't think aria-activedescendant for menus is a good idea. It's still a bit buggy, lends itself far more to selection widgets, and the WAI-ARIA guidelines specifically say items should be focusable). For example: some screen readers still don't read the active item unless it also has aria-selected="true".

There are also some potential interactive and semantic issues related to dropdown/flyout menus. Keyboard interaction is a little more complex, with left/right arrow keys needed on individual menu items to open sub-menus (or up/down, depending on orientation). Any links that open sub-menus would need aria-haspopup and aria-expanded as well.

Right now I think the best possibility would be to pull out the keyboard logic into a utility, since that has potentially broad applicability. Listbox isn’t much more than that logic, some render functions, and scrolling (which Menu wouldn't need).

@dylans dylans removed this from the beta.5 milestone Jan 17, 2018
@dylans dylans removed the beta5 label Jan 17, 2018
@dylans
Copy link
Member

dylans commented Jan 17, 2018

This is most likely going to be post 2.0.

@bitpshr bitpshr removed their assignment Nov 14, 2018
@schontz
Copy link
Contributor

schontz commented Apr 10, 2019

What's the status of the Menu widget? It appears that it died on the vine.

My use case is for a context menu, which looks like it may be supported according to @mwistrand's comment, but it isn't entirely clear. Perhaps there would need to be a ContextMenu widget that could also contain MenuItems, or a Menu even?

@mwistrand
Copy link
Contributor

@schontz I'm not sure how this never made it into either the issue or PR threads, but at the time the menu widget I had worked on was much closer "to the metal" than anyone on the core team was comfortable with (I don't think we had a Focus meta at the time, for example). Further, there are a number of different types of menus, each with their own accessibility requirements. As a result, we decided that one widget to encapsulate all the use cases was never going to be clean or useful. My focus has been on other parts of the framework as of late, but I can at least ask around about what other options/plans are in place.

@schontz
Copy link
Contributor

schontz commented Apr 11, 2019

Sounds good. Is there any kind of widget roadmap/wishlist out there? It may be good to see general interest.

@tomdye
Copy link
Member

tomdye commented Apr 11, 2019

@schontz We hope to work on an implementation of Menu similar to that of the material menu for widgets v7. I plan for this to be a generic menu widget which we can use as the drop down for select / combobox etc as well as in a toolbar or other application navigation. It should be flexible enough to be used as a contextMenu as per your suggested use case.
We're rounding off the features for v6 of widgets right now. You can see the list of changes etc in the v6 changes issue here: #667.
When it comes to designing / developing said widget, I would imagine that a fresh issue will be created and this one closed as it is largely out of date with the current dojo framework.

@tomdye
Copy link
Member

tomdye commented Mar 22, 2021

I believe this can be closed now as we have implemented the Menu / List / Select / TypeAhead

@tomdye tomdye closed this as completed Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests