[MISC] Add Multistate Chip #2148
Conversation
Add a chip that can be clicked to cycle between multiple different values and appearances
| const [ currentStateIndex, setCurrentState ] = useState(0); | ||
|
|
||
| useMemo(() => { | ||
| if (currentStateIndex > states?.length && currentStateIndex > 0) { |
There was a problem hiding this comment.
I am confused here with the reason for adding the second part of expression. Is there a case the currentStateIndex > states?.length but can be currentStateIndex <= 0?
There was a problem hiding this comment.
Indeed the second part seems unnecessary. Any value less than 1 will make the first part of the expression false regardless of what is in states.
| // specific language governing permissions and limitations | ||
| // under the License. | ||
| // | ||
| import React, { useState, useMemo } from "react"; |
There was a problem hiding this comment.
No React import after CARDS-2794
| } | ||
| }, [states]) | ||
|
|
||
| return <Tooltip title={states[currentStateIndex]?.tooltip}> |
There was a problem hiding this comment.
I see we heavily rely on states[currentStateIndex]?. What if there is no states[currentStateIndex] or no states? Should we still attempt to render anything in this case case or abort early?
Also I'd add just to avoid code duplication
const state = states[currentStateIndex];
There was a problem hiding this comment.
We should just make states required and ensure it's non-empty. There's no point in a multistate chip without having the states defined.
| onClick={() => { | ||
| let newIndex = (currentStateIndex + 1 >= states?.length) ? 0 : (currentStateIndex + 1); | ||
| setCurrentState(newIndex); | ||
| onChange(states[newIndex].value); |
There was a problem hiding this comment.
If we are not sure states exists here (from line 79 states?.length) states[newIndex] can thow error, also if we are generally not sure states[currentStateIndex] exists states[newIndex].value can also potentially throw error
There was a problem hiding this comment.
We must be sure that states is well defined before we start rendering.
| label={states[currentStateIndex]?.label} | ||
| variant={states[currentStateIndex]?.variant} | ||
| color={states[currentStateIndex]?.color} | ||
| icon={states[currentStateIndex]?.icon} |
There was a problem hiding this comment.
Also I'd add just to avoid code duplication
const state = states[currentStateIndex];
Or, a less verbose way of using the state props is to define the ChipState visual props as a plain object and expand it in the rendered Chip's props:
| label={states[currentStateIndex]?.label} | |
| variant={states[currentStateIndex]?.variant} | |
| color={states[currentStateIndex]?.color} | |
| icon={states[currentStateIndex]?.icon} | |
| {...states[currentStateIndex]?.chipProps ?? {}} // this would contain label, variant, color, icon |
| /** | ||
| * A 3 state chip supporting the following 3 values and states, each with a configurable tooltip. | ||
| * Expects a single label and size that will apply to all states. | ||
| * 0: A default, outlined, empty circle icon | ||
| * 1: A success color with a checkmark icon | ||
| * -1: An error color with an x icon | ||
| */ | ||
| export function TriStateChip(props) { | ||
| const { size, label, defaultTooltip, positiveTooltip, negativeTooltip, onChange } = props; | ||
| let states = [ | ||
| new ChipState(label, "outlined", "primary", <RadioButtonUncheckedIcon/>, 0, defaultTooltip), | ||
| new ChipState(label, "outlined", "success", <CheckCircleIcon/>, 1, positiveTooltip), | ||
| new ChipState(label, "outlined", "error", <CancelIcon/>, -1, negativeTooltip), | ||
| ] | ||
|
|
||
| return MultiStateChip({ | ||
| "size": size, | ||
| "states": states, | ||
| "onChange": onChange, | ||
| }); | ||
| } |
There was a problem hiding this comment.
This component should be in a separate file.
| color={states[currentStateIndex]?.color} | ||
| icon={states[currentStateIndex]?.icon} | ||
| onClick={() => { | ||
| let newIndex = (currentStateIndex + 1 >= states?.length) ? 0 : (currentStateIndex + 1); |
There was a problem hiding this comment.
| let newIndex = (currentStateIndex + 1 >= states?.length) ? 0 : (currentStateIndex + 1); | |
| let newIndex = (currentStateIndex + 1) % states.length); |
Must make sure that we don't get to this point with undefined or empty states.
| color: PropTypes.string, | ||
| icon: PropTypes.node, | ||
| value: PropTypes.string, | ||
| })) |
There was a problem hiding this comment.
| })) | |
| })).isRequired |
| } | ||
| }, [states]) | ||
|
|
||
| return <Tooltip title={states[currentStateIndex]?.tooltip}> |
There was a problem hiding this comment.
Only render the chip if we have states. Also there will be a warning if for any reason the tooltip is missing, are we making tooltips required, should we allow them to be absent and not render the tooltip in that case (tedious!) or should we us some sort of default?
| return <Tooltip title={states[currentStateIndex]?.tooltip}> | |
| return states?.length > 0 && | |
| <Tooltip title={states[currentStateIndex]?.tooltip ?? "Click to change"}> |
Add a chip that can be clicked to cycle between multiple different values and appearances