-
Notifications
You must be signed in to change notification settings - Fork 6
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
DSD-1740/close dropdown multiselect #1607
Changes from all commits
c382aff
ac6f48a
27e67ec
2bee9f1
7826fcd
ef113fa
9aa13ae
41fd411
1178b00
262a5b1
0bc9a58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -224,16 +224,41 @@ export const Accordion: ChakraComponent< | |
isDefaultOpen ? [0] : [] | ||
); | ||
|
||
// If the accordionData doesn't already contain refs for the panel | ||
// buttons, add them now. | ||
const updatedAccordionData = accordionData.map((item) => ({ | ||
...item, | ||
buttonInteractionRef: item.buttonInteractionRef || React.createRef(), | ||
})); | ||
|
||
const handleKeyDown = (e) => { | ||
// If the 'esc' key is pressed, | ||
if (e.keyCode === 27) { | ||
const focusedPanelIndex = Number(e.target.dataset.index); | ||
// collapse the currently focused panel. | ||
// (Nothing will happen if the currently | ||
// focused panel is already collapsed.) | ||
// If the 'esc' key is pressed, find the panel the | ||
// 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") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious, why the update from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
let focusedPanelIndex; | ||
if (e.target.dataset.index) { | ||
// If the user is focused on an accordion button... | ||
focusedPanelIndex = Number(e.target.dataset.index); | ||
} else { | ||
// If the user is focused on an element within the panel... | ||
focusedPanelIndex = Number( | ||
e.target.closest("[role='region']").id.split("-").pop() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, but also brittle if the DOM and/or id ever changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
); | ||
} | ||
|
||
setExpandedPanels( | ||
expandedPanels.filter((i) => i !== focusedPanelIndex) | ||
); | ||
|
||
// If something *inside* the accordion was in focus and 'esc' was clicked, | ||
// return focus to the accordion panel | ||
if (updatedAccordionData[focusedPanelIndex].buttonInteractionRef) { | ||
updatedAccordionData[ | ||
focusedPanelIndex | ||
].buttonInteractionRef.current.focus(); | ||
} | ||
} | ||
}; | ||
|
||
|
@@ -248,7 +273,7 @@ export const Accordion: ChakraComponent< | |
{...rest} | ||
> | ||
{getElementsFromData( | ||
accordionData, | ||
updatedAccordionData, | ||
ariaLabel, | ||
id, | ||
isAlwaysRendered, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need it for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true but the Perhaps we should reconsider and move all the logic just to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay @EdwinGuzman and @bigfishdesign13, focus should now return to the button when the |
||
|
||
import Accordion from "./../Accordion/Accordion"; | ||
import Button from "./../Button/Button"; | ||
|
@@ -95,7 +95,7 @@ export const MultiSelect: ChakraComponent< | |
|
||
// Create a ref to hold a reference to the accordian button, enabling us | ||
// to programmatically focus it. | ||
const accordianButtonRef: React.RefObject<HTMLDivElement> = | ||
const accordionButtonRef: React.RefObject<HTMLDivElement> = | ||
useRef<HTMLDivElement>(); | ||
const expandToggleButtonRef: React.RefObject<HTMLButtonElement> = | ||
useRef<HTMLButtonElement>(); | ||
|
@@ -364,7 +364,7 @@ export const MultiSelect: ChakraComponent< | |
{ | ||
accordionType: "default", | ||
// Pass the ref for interaction with the accordion button. | ||
buttonInteractionRef: accordianButtonRef, | ||
buttonInteractionRef: accordionButtonRef, | ||
label: accordionLabel, | ||
panel: accordionPanel, | ||
}, | ||
|
@@ -387,7 +387,7 @@ export const MultiSelect: ChakraComponent< | |
selectedItemsString={selectedItemsString} | ||
selectedItemsCount={selectedItemsCount} | ||
onClear={onClear} | ||
accordianButtonRef={accordianButtonRef} | ||
accordionButtonRef={accordionButtonRef} | ||
/> | ||
)} | ||
</Box> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!