Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unexpected behavior from CheckboxCheck with custom children when unchecked #3814

Open
bengry opened this issue May 22, 2024 · 4 comments
Open

Comments

@bengry
Copy link

bengry commented May 22, 2024

In a case where supplying a custom <CheckboxCheck> where you want to render something even when the item is unchecked (e.g. an empty checkbox), this won't work:

<Ariakit.CheckboxCheck
	checked={false} // realistically this value would come from a store of some kind
>
  <MyCheckbox />
</Ariakit.CheckboxCheck>

In this case the checkbox would only render when checked=true.

I tried to work around this using render:

<Ariakit.CheckboxCheck
	checked={false} // realistically this value would come from a store of some kind
	render={<MyCheckbox />}
/>

But then you get duplicated checkmarks when checked={true}.

So I tried this workaround:

<Ariakit.CheckboxCheck
	checked={false} // realistically this value would come from a store of some kind
	render={<MyCheckbox />}
>
{null}
</Ariakit.CheckboxCheck>

Expecting to override the default children and avoid the duplicated checkboxes, but that doesn't work because Ariakit checked if children is truthy, rather than just !== undefined.

This is of course still workaroundable by using a function for the render prop and omitting children, or passing in an empty Fragment as CheckboxCheck's children, but that seems like a hack more than anything.

I'd expect Ariakit to only supply the default SVG checkmark when children are not specified at all, not just when they're falsey.

@diegohaz
Copy link
Member

Hi @bengry! Could you please provide a StackBlitz or CodeSandbox with the code you're trying to implement and its current behavior? This will help determine if CheckboxCheck is indeed the right component for your needs and if this is a bug report or feature request.

@bengry
Copy link
Author

bengry commented May 24, 2024

Hi @bengry! Could you please provide a StackBlitz or CodeSandbox with the code you're trying to implement and its current behavior? This will help determine if CheckboxCheck is indeed the right component for your needs and if this is a bug report or feature request.

Sure, you can see my use-case demoed here.
This is a rough equivalent I think of my production app in this case, copied from the MenuItemCheckbox example in the docs.

What I'm trying to achieve is to render [ ] when the items are not checked, and [x] when they are.
You can play around with the code a bit, but I could not get my wanted behavior to work.

@diegohaz
Copy link
Member

It seems that CheckboxCheck is not suitable for your use case. Simply get the checked state from the menu store and render your custom checkmark without CheckboxCheck:

export const MenuItemCheckbox = forwardRef<
  HTMLDivElement,
  MenuItemCheckboxProps
>(function MenuItemCheckbox(props, ref) {
  const menu = Ariakit.useMenuContext()!;

  const checked = menu.useState((state) => {
    // Ariakit should probably provide an isChecked/useChecked helper to simplify
    // this implementation
    const name = props.name;
    const value = props.value;
    const storeValue = state.values[name];
    if (value == null) return !!storeValue;
    if (!Array.isArray(storeValue)) return false;
    return storeValue.includes(`${value}`);
  })
  
  return (
    <Ariakit.MenuItemCheckbox
      ref={ref}
      {...props}
      className={clsx('menu-item', props.className)}
    >
      <VisualCheckbox checked={checked} />
      {props.children}
    </Ariakit.MenuItemCheckbox>
  );
});

@bengry
Copy link
Author

bengry commented May 25, 2024

It seems that CheckboxCheck is not suitable for your use case. Simply get the checked state from the menu store and render your custom checkmark without CheckboxCheck:

Thanks, this makes sense. Isn't the checked prop that MenuItemCheckbox gets enough?
Or is it not if we use the store to store the value? If that's the reason - the isChecked/useChecked helper you mentioned make sense and would be nice to have, but not crucial. Our current implementation up top of Ariakit relies on the consumer to pass a checked prop explicitly to this item.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants