show check in both cases #2098
show check in both cases #2098
Conversation
bryancunningham-okta
requested review from
KevinGhadyani-Okta,
benschell-okta and
chrispulsinelli-okta
as code owners
| {(internalSelectedValues?.includes(option.value) || | ||
| internalSelectedValues === option.value) && ( |
There was a problem hiding this comment.
I'm fine with this code, but I think it'd be better to make it clear which case would have an array and which wouldn't by using a ternary with isMultiSelect:
| {(internalSelectedValues?.includes(option.value) || | |
| internalSelectedValues === option.value) && ( | |
| {(isMultiSelect ? internalSelectedValues.includes(option.value) : | |
| internalSelectedValues === option.value) && ( |
There was a problem hiding this comment.
@KevinGhadyani-Okta Even in the non-multiselect version the value is sometimes an array. We need to check both cases I think
| @@ -192,7 +192,7 @@ const TextField = forwardRef<HTMLInputElement, TextFieldProps>( | |||
| inputProps={{ | |||
| "aria-errormessage": errorMessageElementId, | |||
| "aria-labelledby": labelElementId, | |||
| "inputmode": inputMode, | |||
| inputmode: inputMode, | |||
There was a problem hiding this comment.
👍 You said this is a Prettier autofix.
bryancunningham-okta
force-pushed
the
bc/OKTA-688591/select-menu-check
branch
from
227bf17 to
f80332a
Compare
| @@ -191,7 +191,7 @@ const TextField = forwardRef<HTMLInputElement, TextFieldProps>( | |||
| "aria-errormessage": errorMessageElementId, | |||
| "aria-labelledby": labelElementId, | |||
| "data-se": testId, | |||
| inputmode: inputMode, | |||
| {!hasMultipleChoices && | ||
| (internalSelectedValues?.includes(option.value) || | ||
| internalSelectedValues === option.value) && ( | ||
| <ListItemSecondaryAction> | ||
| <CheckIcon /> | ||
| </ListItemSecondaryAction> | ||
| )} |
There was a problem hiding this comment.
This is fine. Makes sense based on Figma.
I would like to check with Design to see if we want a checkbox on the left and a checkmark on the right because it's a bit jarring, the difference between the two side-by-side, but when you can only see one at a time, it's probably fine?
I still want to get their opinion now that we've seen this.
| @@ -256,7 +256,6 @@ const Select = < | |||
| }, | |||
| [normalizedOptions] | |||
| ); | |||
|
|
|||
There was a problem hiding this comment.
Can we re-add this newline?
bryancunningham-okta
force-pushed
the
bc/OKTA-688591/select-menu-check
branch
from
f80332a to
4999a06
Compare
OKTA-688591
Summary
Testing & Screenshots