-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
chore: add wds multiselect component #39300
Changes from 6 commits
23ff529
7358341
fb85d1a
b6735c4
00d3a8d
326a0e2
87eb7bf
d616dea
530a1a1
4b43438
ddeda45
7ee36e0
e222ecd
8ee824c
64de1d8
836cae1
25ec88d
6331ef1
6059092
b9a4589
9d486cb
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 |
---|---|---|
|
@@ -388,4 +388,4 @@ | |
"@types/react": "^17.0.2", | ||
"postcss": "8.4.31" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,8 @@ function CalendarHeading( | |
|
||
return ( | ||
<div className={styles.monthYearDropdown}> | ||
<CalendarMonthDropdown state={state} /> | ||
<CalendarYearDropdown state={state} /> | ||
<CalendarMonthDropdown state={state!} /> | ||
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. note: had to buypass this typescript check with there seems to be an issue on spectrum side in terms of types. 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.
Judging by the state types, it can indeed be null. I think it would be better if we handled this case from our side. Otherwise, FE can simply fall in case of state null. May I ask you to use correct types here and here instead of |
||
<CalendarYearDropdown state={state!} /> | ||
</div> | ||
); | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -94,7 +94,7 @@ export const DatePicker = <T extends DateValue>(props: DatePickerProps<T>) => { | |||||||||||||
label="Time" | ||||||||||||||
maxValue={timeMaxValue} | ||||||||||||||
minValue={timeMinValue} | ||||||||||||||
onChange={state.setTimeValue} | ||||||||||||||
onChange={(value) => state.setTimeValue(value!)} | ||||||||||||||
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. Avoid non-null assertion for TimeField value. The non-null assertion ( Consider a safer approach: -onChange={(value) => state.setTimeValue(value!)}
+onChange={(value) => {
+ if (value) {
+ state.setTimeValue(value);
+ }
+}} 📝 Committable suggestion
Suggested change
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. It would also be good to fix this as well, as there may be issues related to this. Do you understand why value can be Anyway, we could at least add a check for null
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. @KelvinOm Yes, the
Your suggestion to add a null check is the right approach to handle these cases safely. I'll add this as a learning for future reviews. ✏️ Learnings added
|
||||||||||||||
placeholderValue={timePlaceholder} | ||||||||||||||
value={state.timeValue} | ||||||||||||||
/> | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export * from "./FieldError"; | ||
export type { FieldErrorProps } from "./types"; | ||
export { default as fieldErrorStyles } from "./styles.module.css"; | ||
KelvinOm marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,2 @@ | ||
export { ListBox } from "./ListBox"; | ||
export { default as listStyles } from "./styles.module.css"; | ||
export { default as listBoxStyles } from "./styles.module.css"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import { | ||
Popover, | ||
listStyles, | ||
listBoxStyles, | ||
useRootContainer, | ||
POPOVER_LIST_BOX_MAX_HEIGHT, | ||
} from "@appsmith/wds"; | ||
|
@@ -26,7 +26,10 @@ export const Menu = (props: MenuProps) => { | |
UNSTABLE_portalContainer={isRootMenu ? root : undefined} | ||
maxHeight={POPOVER_LIST_BOX_MAX_HEIGHT} | ||
> | ||
<HeadlessMenu className={clsx(listStyles.listBox, className)} {...rest}> | ||
<HeadlessMenu | ||
className={clsx(listBoxStyles.listBox, className)} | ||
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. 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. Fixed. |
||
{...rest} | ||
> | ||
{children} | ||
</HeadlessMenu> | ||
</Popover> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export * from "./src"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,178 @@ | ||
import clsx from "clsx"; | ||
import React, { useRef, useState } from "react"; | ||
import { | ||
FieldLabel, | ||
ListBoxItem, | ||
ListBox, | ||
Popover, | ||
TextField, | ||
textInputStyles, | ||
useRootContainer, | ||
type MultiSelectProps, | ||
inputFieldStyles, | ||
selectStyles, | ||
Text, | ||
POPOVER_LIST_BOX_MAX_HEIGHT, | ||
fieldErrorStyles, | ||
} from "@appsmith/wds"; | ||
import { useField } from "react-aria"; | ||
import type { Selection } from "react-aria-components"; | ||
import { setInteractionModality } from "@react-aria/interactions"; | ||
|
||
import styles from "./styles.module.css"; | ||
|
||
import { | ||
DialogTrigger, | ||
UNSTABLE_Autocomplete, | ||
useFilter, | ||
ButtonContext, | ||
} from "react-aria-components"; | ||
import MultiSelectValue from "./MultiSelectValue"; | ||
|
||
const EmptyState = () => { | ||
return ( | ||
<div className={styles.emptyState}> | ||
<Text color="neutral-subtle">No options found</Text> | ||
</div> | ||
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 could just pass className to Text component.
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. Oh yeah..totally slipped my mind. ( AI and its side effects ) |
||
); | ||
}; | ||
|
||
/** | ||
* Note: React aria components does not provide us any mutliselect componennt or hooks for it. | ||
* We are just replicating the behaviour of mutli select component with all available hooks and components. | ||
* Few things are implemented manually like opening the popover on keydown or keyup when the button is focused | ||
* or focusing the trigger on click of label. | ||
* | ||
* This is a temporary solution until we have a mutli select component from react aria components library. | ||
*/ | ||
export const MultiSelect = <T extends { label: string; value: string }>( | ||
props: MultiSelectProps<T>, | ||
) => { | ||
const { | ||
contextualHelp, | ||
defaultSelectedKeys = new Set(), | ||
disabledKeys, | ||
errorMessage, | ||
excludeFromTabOrder, | ||
isDisabled, | ||
isInvalid, | ||
isLoading, | ||
isRequired, | ||
items, | ||
label, | ||
onSelectionChange: onSelectionChangeProp, | ||
placeholder, | ||
selectedKeys: selectedKeysProp, | ||
size, | ||
} = props; | ||
const root = useRootContainer(); | ||
const [_selectedKeys, _setSelectedKeys] = useState<Selection>(); | ||
const selectedKeys = selectedKeysProp ?? _selectedKeys ?? defaultSelectedKeys; | ||
const setSelectedKeys = onSelectionChangeProp ?? _setSelectedKeys; | ||
const { labelProps } = useField(props); | ||
const { contains } = useFilter({ sensitivity: "base" }); | ||
const triggerRef = useRef<HTMLButtonElement>(null); | ||
// Note we have to use controlled state for the popover as we need a custom logic to open the popover | ||
// for the usecase where we need to open the popover on keydown or keyup when the button is focused. | ||
const [isOpen, setOpen] = useState(false); | ||
|
||
const onKeyDown = (e: React.KeyboardEvent) => { | ||
if (e.key === "ArrowDown" || e.key === "ArrowUp") { | ||
setOpen(true); | ||
} | ||
}; | ||
|
||
const filter = (textValue: string, inputValue: string) => | ||
contains(textValue, inputValue); | ||
|
||
return ( | ||
<ButtonContext.Provider value={{ onKeyDown, ref: triggerRef }}> | ||
<div className={inputFieldStyles.field}> | ||
{Boolean(label) && ( | ||
<FieldLabel | ||
{...labelProps} | ||
contextualHelp={contextualHelp} | ||
isDisabled={Boolean(isDisabled) || Boolean(isLoading)} | ||
isRequired={isRequired} | ||
// this is required to imitate the behavior where on click of label, the trigger or input is focused. | ||
// In our select component, this is done by the useSelect hook. Since we don't have that for multi select, | ||
// we are doing this manually here | ||
onClick={() => { | ||
if (triggerRef.current) { | ||
triggerRef.current.focus(); | ||
|
||
setInteractionModality("keyboard"); | ||
} | ||
}} | ||
> | ||
{label} | ||
</FieldLabel> | ||
)} | ||
<div | ||
className={clsx( | ||
textInputStyles.inputGroup, | ||
selectStyles.selectInputGroup, | ||
)} | ||
> | ||
<DialogTrigger isOpen={isOpen} onOpenChange={setOpen}> | ||
<MultiSelectValue | ||
excludeFromTabOrder={excludeFromTabOrder} | ||
isDisabled={Boolean(isDisabled) || Boolean(isLoading)} | ||
isInvalid={isInvalid} | ||
isLoading={isLoading} | ||
items={items} | ||
placeholder={placeholder} | ||
selectedKeys={selectedKeys} | ||
size={size} | ||
triggerRef={triggerRef} | ||
/> | ||
<Popover | ||
UNSTABLE_portalContainer={root} | ||
className={styles.popover} | ||
maxHeight={POPOVER_LIST_BOX_MAX_HEIGHT} | ||
placement="bottom start" | ||
style={ | ||
{ | ||
"--trigger-width": `${triggerRef?.current?.offsetWidth}px`, | ||
} as React.CSSProperties | ||
} | ||
triggerRef={triggerRef} | ||
> | ||
<UNSTABLE_Autocomplete filter={filter}> | ||
<TextField autoFocus className={styles.textField} /> | ||
<ListBox | ||
className={styles.listBox} | ||
disabledKeys={disabledKeys} | ||
items={items} | ||
onSelectionChange={setSelectedKeys} | ||
renderEmptyState={EmptyState} | ||
selectedKeys={selectedKeys} | ||
selectionMode="multiple" | ||
shouldFocusWrap | ||
> | ||
{(item: T) => ( | ||
<ListBoxItem | ||
className={styles.listBoxItem} | ||
id={item.value} | ||
textValue={item.label} | ||
> | ||
{item.label} | ||
</ListBoxItem> | ||
)} | ||
</ListBox> | ||
</UNSTABLE_Autocomplete> | ||
</Popover> | ||
</DialogTrigger> | ||
</div> | ||
{/* We can't use our FieldError component as it only works when used with FieldErrorContext. | ||
We can use it our Select and other inputs because the implementation is abstracted in the react aria components library. | ||
But since for MultiSelect, we don't have component from react-aria, we have to manually render the error message here. */} | ||
<div className={fieldErrorStyles.errorText}> | ||
<Text color="negative" size="caption"> | ||
{errorMessage} | ||
</Text> | ||
</div> | ||
</div> | ||
</ButtonContext.Provider> | ||
); | ||
}; |
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.
We need 1.6.0 version as it has the AutoComplete component that we need in multi select dropdown for filtering list