-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
🪟 🐛 Fix tag input styles #20020
🪟 🐛 Fix tag input styles #20020
Changes from all commits
e3ef3fd
f375684
0e96257
f301026
dc5e73b
a2e0aaa
e5aff74
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 |
---|---|---|
@@ -1,6 +1,6 @@ | ||
import uniqueId from "lodash/uniqueId"; | ||
import { KeyboardEventHandler, useMemo, useState } from "react"; | ||
import { ActionMeta, MultiValue, OnChangeValue } from "react-select"; | ||
import { ActionMeta, GroupBase, MultiValue, OnChangeValue, StylesConfig } from "react-select"; | ||
import CreatableSelect from "react-select/creatable"; | ||
|
||
import styles from "./TagInput.module.scss"; | ||
|
@@ -9,9 +9,8 @@ const components = { | |
DropdownIndicator: null, | ||
}; | ||
|
||
const customStyles = { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- react-select's typing is lacking here | ||
multiValue: (provided: any) => ({ | ||
const customStyles: StylesConfig<Tag, true, GroupBase<Tag>> = { | ||
multiValue: (provided) => ({ | ||
...provided, | ||
maxWidth: "100%", | ||
display: "flex", | ||
|
@@ -20,17 +19,35 @@ const customStyles = { | |
borderRadius: `${styles.borderRadius}`, | ||
paddingLeft: `${styles.paddingLeft}`, | ||
}), | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- same as above | ||
multiValueLabel: (provided: any) => ({ | ||
multiValueLabel: (provided) => ({ | ||
...provided, | ||
color: `${styles.fontColor}`, | ||
fontWeight: 500, | ||
}), | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- same as above | ||
multiValueRemove: (provided: any) => ({ | ||
multiValueRemove: (provided) => ({ | ||
...provided, | ||
borderRadius: `${styles.borderRadius}`, | ||
cursor: "pointer", | ||
}), | ||
clearIndicator: (provided) => ({ | ||
...provided, | ||
cursor: "pointer", | ||
}), | ||
control: (provided, 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. Another thing I noticed while testing locally: this TagInput requires users to type in a value and then hit Enter for that value to be actually added to the field as a tag. If the users just types a value and then clicks away, the value they typed is erased. I feel like this is not a very clear UX that the user has to press Enter in order for their changes to actually be saved, and it would be better if unfocusing the input also added their text as a tag. I think that behavior actually might have existed before we upgraded the react-select version. Could you explore adding that functionality here? Also fine if you'd rather address that in a separate PR. 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 agree, it can be very annoying to a user, especially as it works fine when navigating via keyboard. I didn't find a "supported" way, but I think I came up with an acceptable workaround. 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. Tried it out and I think this looks good! One question:
What do you mean by "it's not default behavior"? Isn't the |
||
const isInvalid = state.selectProps["aria-invalid"]; | ||
const regularBorderColor = isInvalid ? styles.errorBorderColor : "transparent"; | ||
const hoveredBorderColor = isInvalid ? styles.errorHoveredBorderColor : styles.hoveredBorderColor; | ||
return { | ||
...provided, | ||
backgroundColor: styles.inputBackgroundColor, | ||
boxShadow: "none", | ||
borderColor: state.isFocused ? styles.focusedBorderColor : regularBorderColor, | ||
":hover": { | ||
cursor: "text", | ||
borderColor: state.isFocused ? styles.focusedBorderColor : hoveredBorderColor, | ||
}, | ||
}; | ||
}, | ||
}; | ||
|
||
interface Tag { | ||
|
@@ -56,7 +73,7 @@ const generateStringFromTag = (tag: Tag): string => tag.label; | |
|
||
const delimiters = [",", ";"]; | ||
|
||
export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name, disabled, id }) => { | ||
export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name, disabled, id, error }) => { | ||
const tags = useMemo(() => fieldValue.map(generateTagFromString), [fieldValue]); | ||
|
||
// input value is a tag draft | ||
|
@@ -113,13 +130,26 @@ export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name, | |
} | ||
}; | ||
|
||
/** | ||
* Add current input value as new tag when leaving the control. | ||
* This needs to be implemented outside of the onBlur prop of react-select because it's not default behavior. | ||
*/ | ||
const onBlurControl = () => { | ||
if (inputValue) { | ||
onChange([...fieldValue, inputValue]); | ||
setInputValue(""); | ||
} | ||
}; | ||
|
||
return ( | ||
<div data-testid="tag-input"> | ||
<div data-testid="tag-input" onBlur={onBlurControl}> | ||
<CreatableSelect | ||
inputId={id} | ||
name={name} | ||
components={components} | ||
inputValue={inputValue} | ||
placeholder="" | ||
aria-invalid={Boolean(error)} | ||
isClearable | ||
isMulti | ||
onBlur={() => handleDelete} | ||
|
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.
One thing I noticed when testing locally was that when I hovered my cursor over the TagInput it stays as the
default
cursor, whereas when I hover over the other text input fields it changes to atext
cursor. Could we also make this TagInput have atext
cursor, so that it is consistent? (And I feel a text cursor also makes sense here because the user is still expected to type values into the field)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.
good catch, fixed
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.
@flash1293 one more small nit here: when hovering over the
X
close button of one of the tags, could we go back to thedefault
cursor (or maybepointer
, since that seems to be what we use for buttons)? It currently uses the text cursor which looks a little odd.We should also use the same pointer style for the whole-component clear
X
button on the right side