feat: checkbox_group re-export #2212
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2212 +/- ##
==========================================
+ Coverage 46.61% 46.64% +0.03%
==========================================
Files 692 694 +2
Lines 38508 38517 +9
Branches 9826 9804 -22
==========================================
+ Hits 17949 17965 +16
+ Misses 20506 20499 -7
Partials 53 53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if (isElementOfType(child, Checkbox)) { | ||
| return child; | ||
| } | ||
| return <Checkbox checked={false}>{child}</Checkbox>; |
There was a problem hiding this comment.
I think this needs to be the Spectrum Checkbox since that is what we use in dh.ui. That should also allow you to get rid of checked={false}
There was a problem hiding this comment.
Also, I'd still highly recommend adding a couple of representative examples in the Styleguide. As-is, I don't think it actually accepts primitive children. Alternatively, you could write unit tests to verify the JSX is supported and produces expected output.
|
Cannot verify in styleguide as labels are currently blank due to latest core change @bmingles , will update and add examples as soon as that is fixed |
@AkshatJawne I don't think those are related. The labels that are missing are in the ListView right? |
| {'String 2'} | ||
| {444} | ||
| {999} | ||
| {true} |
There was a problem hiding this comment.
Looking at the styleguide, I think there's a bug with handling of true / false
There's probably a stringify that needs to happen somewhere
There was a problem hiding this comment.
When I wrap my children in wrappedChildren, I pass in String(child) into the checkbox, which seems to work for the other primitives. Is there any other way I can stringify?
There was a problem hiding this comment.
I think the issue here is that React.Children.map converts true / false to null.
There was a problem hiding this comment.
You might try ensureArray from @deephaven/utils instead
There was a problem hiding this comment.
Actually, you may still need React.Children.map to deal with key props, but you'll need to convert booleans to strings before passing into it.
| children, | ||
| ...props | ||
| }: CheckboxGroupProps): JSX.Element { | ||
| const [checkedState, setCheckedState] = useState<{ [key: number]: boolean }>( |
There was a problem hiding this comment.
Why do we need to store checkbox state?
There was a problem hiding this comment.
When I use the DH Checkbox, there is this weird effect where I had to store state, since when I was checking one of the checkbox's in the group, all of the checkboxes were going into a checked state.
There was a problem hiding this comment.
So is this unnecessary if you switch to SpectrumCheckbox ?
There was a problem hiding this comment.
I think something like this will work:
const wrappedChildren = useMemo(
() =>
ensureArray(children).map(child =>
isElementOfType(child, Checkbox) ? (
child
) : (
<Checkbox key={String(child)} value={String(child)}>
{String(child)}
</Checkbox>
)
),
[children]
);
|
Ironed out comments, not sure why the String(child) is resulting in booleans resulting in null showing up as the text, @bmingles any ideas? |
|
Should be good now, I still had to clone element, or else when we passed in Checkbox children, then the value would not be assigned properly, and thus selecting one would select all of them. @bmingles |
|
I think the unit test failures are at least in part to needing a rebase / merge on latest main. |
|
@AkshatJawne Code looks good. I think e2e tests may need to be updated. |
|
Attempting to update snapshots right now, running into some difficulty, will update them soon hopefully. |
Resolves #2211
Needed for changes in plugins PR: deephaven/deephaven-plugins#813