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

Improvements to <sl-menu> when used inside of <sl-dropdown> #1688

Closed
claviska opened this issue Oct 26, 2023 · 3 comments
Closed

Improvements to <sl-menu> when used inside of <sl-dropdown> #1688

claviska opened this issue Oct 26, 2023 · 3 comments
Assignees

Comments

@claviska
Copy link
Member

claviska commented Oct 26, 2023

This is a 3.0 milestone because it will likely result in breaking changes. Some valid concerns have been raised about dropdown menus. Here are some of the open questions:

Does it make sense to combine <sl-dropdown> and <sl-menu>?

Dropdown was created at a time before we have <sl-popup> which handles all the position and whatnot. It's primary use case is as a dropdown menu, but it can be used as a low-level component with any contents. This flexibility is nice, but leaves room for user error, especially in terms of accessibility. Slimming down the use case would let us consolidate dropdown, menu, menu item, and menu label into fewer components.

<sl-dropdown>
  <sl-dropdown-item>Item 1</sl-dropdown-item>
  <sl-dropdown-item>Item 2</sl-dropdown-item>
  <sl-dropdown-item>Item 3</sl-dropdown-item>
</sl-dropdown>

This would effectively eliminate menu and menu item, which people frequently mistake for nav menus.

We can optionally group them with a separate component like <optgroup>:

<sl-dropdown>
  <sl-dropdown-group label="Group 1">
    <sl-dropdown-item>Item 1</sl-dropdown-item>
    <sl-dropdown-item>Item 2</sl-dropdown-item>
  </sl-dropdown-group>

  <sl-dropdown-group label="Group 2">
    <sl-dropdown-item>Item 3</sl-dropdown-item>
    <sl-dropdown-item>Item 4</sl-dropdown-item>
  </sl-dropdown-group>
</sl-dropdown>

This would make it much easier to support menuitemcheckbox and menuitemradio controls inside dropdown menus accessibly.

Menu Items need to not steal focus on hover

Another issue we have today is that menu items steal focus when hovering. This messes with the browser's :focus-visible heuristics and causes issues such as #1676. It also causes problems when trying to use dropdown + menu items along with a search filter, as focus is unexpectedly removed on hover. It's worth noting that we could probably build in an accessible search filter if we were to combine these components, which has been requested a number of times.

Given the way menus and menu items currently work, I don't see a way to solve this without a breaking change. (Granted, I haven't spent a ton of time on it either.)

I believe making menu items direct children of the dropdown would make this easier to solve.


If you have any thoughts, feedback, or concerns please post them below.

@claviska claviska added this to the 3.0 (future) milestone Oct 26, 2023
@claviska claviska self-assigned this Oct 26, 2023
@SabineWren
Copy link

SabineWren commented Oct 26, 2023

I like the direction this is going, but have some concerns:

  1. One of the reasons to use sl-dropdown instead of sl-select is performance when rendering many of them (ex. table rows). If a new dropdown replaces the old one, then extra bells and whistles like a search filter risk degrading performance when not used.
  2. Search filters can use related fields instead of the option labels (ex. name OR email).
  3. The dropdown might apply a filter from an external sl-input.
  4. The main constraints with sl-dropdown relate to the options slotted. Maybe allow custom options? Fire sl-select when clicking an element inside a menu-item #1599 (comment)
Filter inside dropdown
<sl-dropdown placement="bottom-end" class="labelled"
   ?disabled=${optionsAll.length <= 1}
   @sl-after-show=${() =>
      // Focusing immediately doesn't work, so we wait for animation
      $<SlInput>(this, `sl-input[type="search"]`).focus()}
   @sl-select=${(e: CustomEvent) => {
      const toSelect = optionsAll.find(r => r.Id === e.detail.item.OrgId)
      // Shoelace enables a checkbox icon only when type="checkbox", but it mutates when clicked.
      // We can circumvent mutation by using live directive and forcing an update
      if (!toSelect || toSelect.Id === selected?.Id) this.requestUpdate()
      else void this.listOrg.ChangeOrganization(toSelect.Id)
   }}>
   <sl-button variant="primary" size="large" slot="trigger" class="rectangle">
      <span class="text-selected">${selected?.Name}</span>
      <span class="text-label">${T.Dropdown.Org}</span>
      <sl-icon slot="suffix" name="caret-down-fill" library="bs"></sl-icon>
   </sl-button>
   ${when(
      this.searchFilter.length > 0 || optionsAll.length > 5,
      () => html`
   <sl-input type="search" clearable .value=${this.searchFilter}
      placeholder="${this.locale.English(`Filter Organizations`)}"
      @sl-input=${(e: Event) => {
         this.searchFilter = (e.currentTarget as SlInput).value
      }}
      ><sl-icon slot="prefix" name="magnifying-glass" library="fa"></sl-icon>
   </sl-input>
   `,
   )}
   <sl-menu>
      ${optionsAll
         .filter(r => r.Id === this.listOrg.Id || r.Name.toLowerCase().includes(this.searchFilter.toLowerCase()))
         .map(
            o => html`
      <sl-menu-item type="checkbox" ?checked=${live(o.Id === selected?.Id)}
         .OrgId=${o.Id}>${o.Name}
      </sl-menu-item>
      `,
         )}
   </sl-menu>
</sl-dropdown>

Screenshot from 2023-10-26 13-20-03

Filter outside dropdown, using some kludgey auto-focus code

override firstUpdated = () => {
   const x = $<SlInput>(this, `sl-input[type="search"]`)
   x.hasUpdated ? x.focus() : x.updateComplete.then(() => x.focus())
}

Screenshot from 2023-10-26 13-28-19

@SabineWren
Copy link

SabineWren commented Oct 26, 2023

Would love to see other use cases ☺️

Update:
Related autocomplete #489
Related combobox #1648

@claviska
Copy link
Member Author

I'm closing this since we're tracking it elsewhere now.

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