Add simple CycleButton and use it in three way checkboxes #1296
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| extends Omit<ActionableButtonProps, "primitiveType"> { | ||
| noState?: boolean; | ||
| value?: string; | ||
| onInterract?: () => void; |
There was a problem hiding this comment.
The prop name onInterract contains a typo and should be spelled onInteract (single 't'). This change would align with standard React naming conventions and correct spelling.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| onInterract(); | ||
| } | ||
| }} | ||
| onChange={onValueChange ? () => onValueChange(currentValue) : undefined} |
There was a problem hiding this comment.
The onChange handler should be moved inside the onClick handler and called after setCurrentValue. Since setCurrentValue is asynchronous, the current implementation will pass the old value to onValueChange. Moving it after setCurrentValue ensures the new value is passed correctly.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Oksamies
force-pushed
the
01-20-add_simple_cyclebutton_and_use_it_in_three_way_checkboxes
branch
from
9502128 to
0c5e861
Compare
| onCheckedChange={() => toggleCategory(c.id)} | ||
| className={styles.checkbox} | ||
| <CycleButton | ||
| onInterract={() => toggleCategory(c.id)} |
There was a problem hiding this comment.
The prop name onInterract contains a typo and should be onInteract to match the component's interface definition. This will prevent TypeScript errors and maintain consistency with the CycleButton component's API.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
| onClick={() => { | ||
| if (options && options.length > 0) { | ||
| const currentValueIndex = options?.indexOf(currentValue); | ||
| if ( | ||
| currentValueIndex === -1 || | ||
| currentValueIndex === options.length - 1 | ||
| ) { | ||
| setCurrentValue(options[0]); | ||
| } else { | ||
| setCurrentValue(options[currentValueIndex + 1]); | ||
| } | ||
| } | ||
| if (onInteract) { | ||
| onInteract(); | ||
| } | ||
| if (onValueChange) { | ||
| onValueChange(currentValue); | ||
| } | ||
| }} |
There was a problem hiding this comment.
The onValueChange callback is receiving the previous state value because currentValue hasn't been updated when the callback runs. To fix this, calculate the next value once, then use it for both setCurrentValue and onValueChange. This ensures the callback receives the correct new value.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Oksamies
force-pushed
the
01-20-add_simple_cyclebutton_and_use_it_in_three_way_checkboxes
branch
from
0c5e861 to
3cb6694
Compare
| let newValue = ""; | ||
| if (options && options.length > 0) { | ||
| const currentValueIndex = options?.indexOf(currentValue); | ||
| if ( | ||
| currentValueIndex === -1 || | ||
| currentValueIndex === options.length - 1 | ||
| ) { | ||
| newValue = options[0]; | ||
| } else { | ||
| newValue = options[currentValueIndex + 1]; | ||
| } | ||
| } |
There was a problem hiding this comment.
The newValue calculation should preserve currentValue when options is undefined or empty, rather than defaulting to an empty string. Currently, the code will set newValue = "" in these cases, which could cause unexpected state changes. Consider guarding this logic:
let newValue = currentValue;
if (options?.length > 0) {
// existing cycling logic
}Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Oksamies
force-pushed
the
01-20-add_simple_cyclebutton_and_use_it_in_three_way_checkboxes
branch
from
3cb6694 to
94efb63
Compare
Oksamies
deleted the
01-20-add_simple_cyclebutton_and_use_it_in_three_way_checkboxes
branch
No description provided.