Add a multi-value input component #47804
Conversation
bl-nero
added
no-changelog
| <Fieldset> | ||
| {label && <Legend>{label}</Legend>} | ||
| {value.map((val, i) => ( | ||
| // Note on keys: using index as a key is an anti-pattern in general, |
There was a problem hiding this comment.
// Note on keys: using index as a key is an anti-pattern in general,
TIL 👍
| if (value.length === 0) { | ||
| value = ['']; | ||
| } |
There was a problem hiding this comment.
This doesn't seem right. I don't like updating the prop value here directly, especially with an array of length 1. Because arrays are passed by reference, and this is probably provided by some state, this is technically mutating state directly which we shouldn't do in react.
Can we think of another way to do this?
There was a problem hiding this comment.
It doesn't work this way: if you pass an array by to a function (or set it to a variable), then reassigning to it only changes the local reference, and leaves the original object alone. However, I get it that it may look ugly, so I can just introduce a new variable.
There was a problem hiding this comment.
(What would be a violation of rules would be saying value[0] = ''.)
| // synthetically and injected from outside (which would make the API | ||
| // difficult to use) or to keep the array with generated IDs as local | ||
| // state (which would require us to write a prop/state reconciliation | ||
| // procedure whose complexity would probably outweigh the benefits). |
| onClick={() => removeItem(i)} | ||
| disabled={disabled} | ||
| > | ||
| <Icon.Cross size="small" color={theme.colors.text.muted} /> |
There was a problem hiding this comment.
Love the cross here instead of trashcan, thank you!
| onClick={() => insertItem(value.length)} | ||
| > | ||
| <Icon.Plus size="small" mr={2} /> | ||
| Add More |
There was a problem hiding this comment.
Related to my comment about the empty value, perhaps we don't have any input and if length === 0, this says "add field" and then "add more" if greater than 1? Idk, spitballin here.
There was a problem hiding this comment.
The way I wrote it, I assumed that there's always at least 1 input element, and if it's empty, it simply means an empty array. Therefore there's no case where length === 0. It's an arbitrary decision, I know; it just felt right for the time being, since it's easiest to populate the input (no need to click a button). I know, however, that it leaves an edge case that is unsupported, which is a single empty string. It wasn't really important for the job I was doing at the moment; perhaps if we use this component in more places, we can toggle this behavior with a boolean flag.
I created this component to handle sets of long strings that make use of
FieldSelectCreateimpractical. The first use of it will be the app access section of the role editor, which will need to accommodate Azure identity IDs. These strings are very long and can't be effectively dealt with byFieldSelectCreate, which is our current go-to solution for editing collections of strings.The design borrows both from existing label editors, but uses the "X" symbol for removing rows to be consistent with the new designs. Further updates will also use the same icon for label removal.
Also pending: making the "add more" buttons smaller across the board in both this and label editor components. This is now kept as is to maintain consistency with other UIs so far.
This change depends on #47674, but only because it's a part of a bigger PR chain. It's self-contained, so it should be easy to backport.
Followed up by #47803.