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

Feature: Multiple selection #202

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

fajar-apri-alaska
Copy link
Contributor

@fajar-apri-alaska fajar-apri-alaska commented May 14, 2024

Alaska Airlines Pull Request

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Resolves: #199

Summary:

The code diff introduces a new multiSelect attribute to the AuroMenu component, allowing for multiple options to be selected. Here's a breakdown of the functional differences:

  1. New Attribute: multiSelect

    • Definition: Added a multiSelect Boolean attribute to the class, allowing the menu to support multiple selections.
    • Usage:
      this.multiSelect = undefined;
  2. Handling value Property

    • Before: The value property was a single string representing the selected value.
    • After: The value property can be either a string or an array of strings when multiSelect is true.
      if (!this.multiSelect) {
        this.selectByValue(this.value);
      } else {
        const values = JSON.parse(this.value);
        if (this.value && Array.isArray(values)) {
          values.forEach((val) => {
            this.selectByValue(val);
          });
        }
      }
  3. Selection State Management

    • New Methods: Added methods to handle multiple selections:
      • toggleSelectionState: Toggles the selection state of an option.
      • handleDeselectState: Handles deselection of an option, updating the value array.
      • Modified handleLocalSelectState to support multiple selections by updating the value array.
      toggleSelectionState(option) {
        if (option.hasAttribute('selected')) {
          this.handleDeselectState(option);
        } else {
          this.handleLocalSelectState(option);
        }
      }
      handleDeselectState(option) {
        option.removeAttribute('selected');
        option.classList.remove('active');
        option.ariaSelected = false;
      
        if (this.multiSelect && Array.isArray(this.value)) {
          this.value = this.value.filter((val) => val !== option.value);
          if (this.value.length === 0) {
            this.value = undefined;
          }
        }
      
        if (this.multiSelect && Array.isArray(this.optionSelected)) {
          this.optionSelected = this.optionSelected.filter((val) => val !== option);
          if (this.optionSelected.length === 0) {
            this.optionSelected = undefined;
          }
        }
      
        this.index = this.items.indexOf(option);
      }
  4. Resetting Option States

    • Before: resetOptionsStates was always called when selecting an item.
    • After: resetOptionsStates is only called when multiSelect is false.
      if (!this.multiSelect) {
        this.resetOptionsStates();
      }
  5. Event Handling

    • Modified event handling to toggle selection state instead of always setting it:
      if (!this.multiSelect) {
        this.handleLocalSelectState(option);
      } else {
        this.toggleSelectionState(option);
      }

The changes allow the AuroMenu component to handle multiple selections by using an array for the value property and providing methods to manage the selection and deselection of multiple options.

Type of change:

Please delete options that are not relevant.

  • New capability
  • Revision of an existing capability
  • Infrastructure change (automation, etc.)
  • Other (please elaborate)

Checklist:

  • My update follows the CONTRIBUTING guidelines of this project
  • I have performed a self-review of my own update

By submitting this Pull Request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Pull Requests will be evaluated by their quality of update and whether it is consistent with the goals and values of this project. Any submission is to be considered a conversation between the submitter and the maintainers of this project and may require changes to your submission.

Thank you for your submission!

-- Auro Design System Team

@fajar-apri-alaska fajar-apri-alaska self-assigned this May 14, 2024
@fajar-apri-alaska fajar-apri-alaska requested a review from a team as a code owner May 14, 2024 11:11
@fajar-apri-alaska fajar-apri-alaska linked an issue May 14, 2024 that may be closed by this pull request
docs/partials/api.md Outdated Show resolved Hide resolved
src/auro-menu.js Outdated Show resolved Hide resolved
src/auro-menu.js Outdated Show resolved Hide resolved
src/auro-menu.js Outdated Show resolved Hide resolved
src/auro-menu.js Outdated Show resolved Hide resolved
src/auro-menu.js Outdated Show resolved Hide resolved
test/auro-menu.test.js Outdated Show resolved Hide resolved
test/auro-menu.test.js Outdated Show resolved Hide resolved
test/auro-menu.test.js Outdated Show resolved Hide resolved
test/auro-menu.test.js Outdated Show resolved Hide resolved
@blackfalcon
Copy link
Member

While this is functionally living up to expectations, we have a pretty big a11y/UX issue. Please see the attached video. Now that we have this option to multi-select more than one menu item, using the keyboard arrow keys to move up and down the list to unselect any options is almost impossible.

See in the video that the DOM is triggering active state, but the UI is NOT reflecting this feedback making this almost unseable to assistive device use.

@leeejune we will need some assistance with this.

May-14-2024 12-43-19

@leeejune
Copy link
Contributor

leeejune commented May 14, 2024

@blackfalcon I see a few suggestions to address this:

  1. We use checkboxes for multi select menu items. I don't know how the behavior is inherited, but I see the "press space to select" behavior being inherited. That should solve this issue of selecting with up/down arrows.
  2. We implement the "press enter to select" behavior like how it is shown here: https://semantic-ui.com/modules/dropdown.html

@blackfalcon
Copy link
Member

blackfalcon commented May 20, 2024

@blackfalcon I see a few suggestions to address this:

  1. We use checkboxes for multi select menu items. I don't know how the behavior is inherited, but I see the "press space to select" behavior being inherited. That should solve this issue of selecting with up/down arrows.
  2. We implement the "press enter to select" behavior like how it is shown here: https://semantic-ui.com/modules/dropdown.html

Some of the recommendations you are suggesting will be implemented with the new multi-select combobox element where the selected options will appear in a list above the dropdown. But that's not the issue I am bringing up here.

I am electing to move this to a new issue where we can discuss a keyboard interaction solution.

#203

@blackfalcon blackfalcon force-pushed the fajarapri/multiple branch 3 times, most recently from f4ba4a4 to eef024c Compare May 20, 2024 23:50
Copy link
Member

@blackfalcon blackfalcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this commit and PR to fit an evolving specification the team is going for. Please let me know if you have any questions.

Also, please look into this weird warning coming from the test logs on a build

https://github.com/AlaskaAirlines/auro-menu/actions/runs/9166453632/job/25201867531#step:6:25

@fajar-apri-alaska
Copy link
Contributor Author

I updated this commit and PR to fit an evolving specification the team is going for. Please let me know if you have any questions.

Also, please look into this weird warning coming from the test logs on a build

https://github.com/AlaskaAirlines/auro-menu/actions/runs/9166453632/job/25201867531#step:6:25

I am aware of this as well.
Since there is a part of the code where it reads the value attributes (which it can be an array). So I am putting that inside try catch function to avoid function break.

@jason-capsule42
Copy link
Member

jason-capsule42 commented Aug 20, 2024

@sourcery-ai review

The core of this commit is updating the ability of the element to know
the difference between the default single-select and the multi-select
option. With the multi-select options selected, the value of the menu
selections will be kept in an array versus a single string.

Changes to be committed:
modified:   src/auro-menu.js
modified:   src/style-base.scss
modified:   src/style-menuoption.scss
Changes to be committed:
new file:   apiExamples/multiple.html
modified:   docs/partials/api.md
A reference string in the page was removed as it's only template
content.

Changes to be committed:
modified:   docs/partials/index.md
modified:   demo/index.md
This commit updates the pakcage.json build scripts to be compliant with
the decisions made in this discussion:

https://github.com/orgs/AlaskaAirlines/discussions/523

This commit also updates the `assets` object to reference the correct
api.md file for post-back to the repo.

Changes to be committed:
modified:   .github/workflows/testPublish.yml
modified:   package.json
Changes to be committed:
modified:   package-lock.json
modified:   package.json
Changes to be committed:
modified:   test/auro-menu.test.js
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

Successfully merging this pull request may close these issues.

auro-menu: multiselect
4 participants