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

Web and Native discrepancies #26

Open
ianmartorell opened this issue Aug 15, 2024 · 4 comments
Open

Web and Native discrepancies #26

ianmartorell opened this issue Aug 15, 2024 · 4 comments

Comments

@ianmartorell
Copy link

ianmartorell commented Aug 15, 2024

Why is it that Select's onValueChange behaves differently in web and native? I see that Radix only passes the value to the callback function but it's a bit confusing that the value gets passed as the label as well to onValueChange. There's no mention of it in the documentation either, I thought it was a bug.

/**
* @web Parameter of `onValueChange` has the value of `value` for the `value` and the `label` of the selected Option
* @ex When an Option with a label of Green Apple, the parameter passed to `onValueChange` is { value: 'green-apple', label: 'green-apple' }
*/

Would you consider unifying the API by either finding the label in the web version to pass it to the callback, or changing the API of the native primitive?

Is having parity between platforms a goal of this project?

Thank you!

@kilbot
Copy link

kilbot commented Sep 5, 2024

I agree with this, it seems an unnecessary complication.

@mattddean
Copy link

I think this can be addressed by matching the Radix API. Radix is the standard. Whether or not it's more convenient to receive the entire option back, anyone who is trying to migrate from Radix will have an easier time if the API exactly matches. onValueChange's signature ought to be (value: string) => void and value ought to be a string IMO.

@mattddean
Copy link

@mrzachnugent i would love to submit a PR to do this with your blessing

@mrzachnugent
Copy link
Collaborator

@ianmartorell PR are more than welcome!

If I remember correctly, this added complexity is to allow the native and web components to have as much freedom. I think the main issue was with getting the child of Select.ItemText to show in the Select.Value component on native.

If you could unify the API while maintaining the flexibility of having something like the following snippet, that would be awesome.

const SelectItem = React.forwardRef(({ children, className, ...props }, forwardedRef) => {
  return (
    <Select.Item className={classnames('SelectItem', className)} {...props} ref={forwardedRef}>
      <Select.ItemText>{children}</Select.ItemText>
      <Select.ItemIndicator className="SelectItemIndicator">
        <CheckIcon />
      </Select.ItemIndicator>
    </Select.Item>
  );
});

The flexibility comes from being able to add more than just text in the Select.Item in a composable manner.

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

4 participants