DSD-1740/close dropdown multiselect #1607

oliviawongnyc · 2024-05-22T21:37:11Z

Fixes JIRA ticket DSD-1740

This PR does the following:

Adds the ability to close the Accordion if an element within the panel is focused (for example, if a checkbox within the MultiSelect is focused, this PR will close the parent panel when the user presses esc).

How has this been tested?

Locally, in Storybook.
Also added another unit test.

Accessibility concerns or updates

None.

Checklist:

I have updated the Storybook documentation accordingly.
I have added relevant accessibility documentation for this pull request.
All new and existing tests passed.

Front End Review:

Review the Vercel preview deployment once it is ready.

vercel · 2024-05-22T21:37:15Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nypl-design-system	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 24, 2024 5:52pm

EdwinGuzman

I think your tests may break based on my comment so will review that once this is updated.

EdwinGuzman · 2024-05-22T21:51:27Z

src/components/Accordion/accordionChangelogData.ts

@@ -14,7 +14,18 @@ export const changelogData: ChangelogData[] = [
    version: "Prerelease",
    type: "Update",
    affects: ["Accessibility", "Functionality"],
-    notes: ["Adds logic to close accordion when 'esc' key is pressed"],
+    notes: [
+      "Adds logic to close accordion when element within panel is focused and 'esc' key is pressed",


This is duplicated below.

They're slightly different, but I can combine them if you prefer.

You can keep both but what I mean is you only need one object with "Prerelease" but now there are two. Unless if this was done because you don't think this will get deployed by 5/23. If that's the case then it's okay to keep both.

I see! Okay. Will combine.

EdwinGuzman · 2024-05-22T21:54:41Z

src/components/Accordion/Accordion.tsx

+          setExpandedPanels(
+            expandedPanels.filter((i) => i !== focusedPanelIndex)
+          );


This is within the else block and it doesn't seem intentional because the esc key should still be functional when the user is focused on the accordion button.

Yes! Good catch. Thank you.

EdwinGuzman · 2024-05-22T21:55:58Z

src/components/Accordion/Accordion.tsx

+        } else {
+          // If the user is focused on an element within the panel...
+          focusedPanelIndex = Number(
+            e.target.closest("[role='region']").id.split("-").pop()


Nice, but also brittle if the DOM and/or id ever changes.

I know :/, I didn't want to do it this way, but it seemed like the best option...the id is really the only way the panel is connected to the button that needs to close.

EdwinGuzman · 2024-05-23T13:52:11Z

src/components/Accordion/Accordion.test.tsx

@@ -160,19 +160,17 @@ describe("Accordion", () => {
  });

  it("closes the accordion when the 'esc' key is pressed", async () => {
-    render(<Accordion accordionData={[accordionData[0]]} isDefaultOpen />);
+    render(<Accordion accordionData={[accordionData[0]]} />);


Can you add another test where the entire accordionData array is passed, so now there are three accordions. For one of the accordions, add a button or checkbox, focus on it, and then press escape. This is to test the new feature in this PR but also that it works when there are more than just one accordion.

I know there's a test that does this for MultiSelect but the feature is in the Accordion so it should also be tested there.

bigfishdesign13

Please see my comment about how focus should be handled.

bigfishdesign13 · 2024-05-23T13:53:20Z

src/components/MultiSelect/MultiSelect.tsx

@@ -4,7 +4,7 @@ import {
  ChakraComponent,
  useMultiStyleConfig,
 } from "@chakra-ui/react";
-import React, { useState, forwardRef, useRef } from "react";
+import React, { forwardRef, useRef, useState } from "react";


I was not sure where to put this comment, but the focus needs to be addressed after the ESC key is pressed when a checkbox has focus. In that scenario, the focus should return to the main button after the ESC key is pressed.

That's a good point. If it involves having state or ref we should talk about this because it's getting overly complex and from what I understood, accordions don't generally behave this way. So if we don't have to implement this then perhaps we should not.

We don't need it for Accordion, but we do need it for MultiSelect.

That's true but the Accordion is a big part of the MultiSelect and, to me, MultiSelect is inheriting all the features of Accordion. There is currently a ref that is being passed from the MultiSelect to the Accordion to keep track of focus for when the items count button is clicked. This could be used as well when the esc key is pressed. But it we have to keep track of pressing the ESC key in the MultiSelect to close the Accordion, then it feels like the work is being duplicated because the Accordion already does check for when the ESC key is pressed.

Perhaps we should reconsider and move all the logic just to the MultiSelect from the Accordion. @oliviawongnyc As you try to implement the focus management in the Accordion, let me know if my comment makes sense.

Okay @EdwinGuzman and @bigfishdesign13, focus should now return to the button when the Accordion (or MultiSelect) is closed with the esc key.

EdwinGuzman

Update looks good but tests are failing

bigfishdesign13

The esc focus handling works great in MultiSelect, but it is not working in Accordion.

EdwinGuzman

The update looks good to me and it works in both the Accordion and MultiSelect.

EdwinGuzman · 2024-06-12T19:12:07Z

src/components/Accordion/Accordion.tsx

+      // user is focused on or within, and remove it as
+      // an expanded panel. (Nothing will happen if the
+      // panel is already collapsed.)
+      if (e.code === "Escape") {


Just curious, why the update from e.keyCode to e.code?

Two reasons. 1) I thought it was clearer than. using a keyCode, and 2) for some reason in the tests, the keyCode for Escape was a different number than it is in the code? So I used the code 'Escape' instead for the tests and wanted to be consistent.

EdwinGuzman

Really nice refactor in the last commit.

bigfishdesign13

Awesome!

oliviawongnyc added 4 commits May 22, 2024 09:41

Adds keydown listener

c382aff

Merge branch 'development' into DSD-1740/close-dropdown-multiselect

ac6f48a

Adds logic to close accordion when inner element is focused

27e67ec

Merges in development

2bee9f1

vercel bot deployed to Preview May 22, 2024 21:37 View deployment

oliviawongnyc requested review from EdwinGuzman and bigfishdesign13 May 22, 2024 21:37

oliviawongnyc added the Needs Review Pull requests that are ready for peer review. label May 22, 2024

EdwinGuzman requested changes May 22, 2024

View reviewed changes

Updates per PR feedback

7826fcd

vercel bot deployed to Preview May 23, 2024 13:26 View deployment

EdwinGuzman reviewed May 23, 2024

View reviewed changes

bigfishdesign13 requested changes May 23, 2024

View reviewed changes

bigfishdesign13 added the Changes Requested Pull requests where changes have been requested in peer review. label May 23, 2024

Updates per PR feedback

ef113fa

vercel bot deployed to Preview May 24, 2024 14:05 View deployment

EdwinGuzman reviewed May 24, 2024

View reviewed changes

Fixes tests

9aa13ae

vercel bot deployed to Preview May 24, 2024 15:27 View deployment

Updates per feedback

41fd411

vercel bot deployed to Preview May 29, 2024 17:33 View deployment

Updates documentation

1178b00

oliviawongnyc requested review from EdwinGuzman and bigfishdesign13 May 29, 2024 17:35

vercel bot deployed to Preview May 29, 2024 17:38 View deployment

bigfishdesign13 reviewed Jun 6, 2024

View reviewed changes

bigfishdesign13 added the x2 This PR needs at least two approvals. label Jun 6, 2024

EdwinGuzman approved these changes Jun 12, 2024

View reviewed changes

Merge branch 'development' into DSD-1740/close-dropdown-multiselect

262a5b1

Updates per PR feedback

0bc9a58

vercel bot deployed to Preview June 24, 2024 17:52 View deployment

oliviawongnyc requested review from bigfishdesign13 and EdwinGuzman June 24, 2024 18:07

EdwinGuzman approved these changes Jun 24, 2024

View reviewed changes

EdwinGuzman removed the Changes Requested Pull requests where changes have been requested in peer review. label Jun 24, 2024

bigfishdesign13 approved these changes Jun 24, 2024

View reviewed changes

bigfishdesign13 added Ship It Pull requests that have been reviewed and approved. and removed Needs Review Pull requests that are ready for peer review. x2 This PR needs at least two approvals. labels Jun 24, 2024

oliviawongnyc merged commit bae5120 into development Jun 25, 2024
5 checks passed

oliviawongnyc deleted the DSD-1740/close-dropdown-multiselect branch June 25, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DSD-1740/close dropdown multiselect #1607

DSD-1740/close dropdown multiselect #1607

oliviawongnyc commented May 22, 2024

vercel bot commented May 22, 2024 •

edited

Loading

EdwinGuzman left a comment

EdwinGuzman May 22, 2024

oliviawongnyc May 23, 2024

EdwinGuzman May 23, 2024

oliviawongnyc May 24, 2024

EdwinGuzman May 22, 2024

oliviawongnyc May 23, 2024

EdwinGuzman May 22, 2024

oliviawongnyc May 23, 2024

EdwinGuzman May 23, 2024

EdwinGuzman May 23, 2024

oliviawongnyc May 24, 2024

bigfishdesign13 left a comment •

edited

Loading

bigfishdesign13 May 23, 2024

EdwinGuzman May 23, 2024

bigfishdesign13 May 23, 2024

EdwinGuzman May 23, 2024

oliviawongnyc May 29, 2024

EdwinGuzman left a comment

bigfishdesign13 left a comment

EdwinGuzman left a comment

EdwinGuzman Jun 12, 2024

oliviawongnyc Jun 24, 2024

EdwinGuzman left a comment

bigfishdesign13 left a comment

DSD-1740/close dropdown multiselect #1607

DSD-1740/close dropdown multiselect #1607

Conversation

oliviawongnyc commented May 22, 2024

This PR does the following:

How has this been tested?

Accessibility concerns or updates

Checklist:

Front End Review:

vercel bot commented May 22, 2024 • edited Loading

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigfishdesign13 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

bigfishdesign13 left a comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EdwinGuzman left a comment

Choose a reason for hiding this comment

bigfishdesign13 left a comment

Choose a reason for hiding this comment

vercel bot commented May 22, 2024 •

edited

Loading

bigfishdesign13 left a comment •

edited

Loading