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

Should we support <select /> with multiple=true like ReactJS? #283

Open
p1xelHer0 opened this issue Oct 4, 2018 · 4 comments
Open

Should we support <select /> with multiple=true like ReactJS? #283

p1xelHer0 opened this issue Oct 4, 2018 · 4 comments
Labels
docs issues that are about providing better documentation question RFC

Comments

@p1xelHer0
Copy link

In ReactJS we can easily make a multiselect component by setting our value to an array of strings instead of a single string like so:

Note

You can pass an array into the value attribute, allowing you to select multiple options in a select tag:

<select multiple={true} value={['B', 'C']}>

see: https://reactjs.org/docs/forms.html#the-select-tag

Intuitively, I thought this would be the case for ReasonReact as well, allowing us to use list(string) or array(string) to achieve the same behavior.

As of now the value of a <select /> component expects value to be a string, even when multiple=true. adding multiple=true and value=string satisfies the compiler, but we'll get an error in the browser like so:

Warning: The `value` prop supplied to <select> must be an array if `multiple` is true.

Check the render method of `Multiselect`.
    in select (created by Multiselect)
    in div (created by Multiselect)
    in Multiselect

Now, if we try to obey the JS error we get the following error in Reason:

This expression has type string but an expression was expected of type list(string)

It would be helpful to be able to handle this with list(string) or array(string), just like ReactJS does it.

@p1xelHer0
Copy link
Author

Here is an example application which showcases this, where we can comment/uncomment the lines to switch between the behavior of multiple=true:

type state = {
  selectedItems: list(string),
  /* selectedItems: string, */
  items: list(string),
};

type action =
  | SelectItem(string);

let component = ReasonReact.reducerComponent("Multiselect");

let make = _children => {
  ...component,
  initialState: () => {
    items: ["a", "b", "c", "f", "g", "x"],
    selectedItems: [],
    /* selectedItems: "", */
  },
  reducer: (action, state) =>
    switch (action) {
    /* | SelectItem(item) => ReasonReact.Update({...state, selectedItems: item}) */
    | SelectItem(item) =>
      ReasonReact.Update({...state, selectedItems: [item]})
    },
  render: self =>
    <div>
      <h1>
        {ReasonReact.string("Selected item: ")}
        /* {ReasonReact.string(self.state.selectedItems)} */
        {
          self.state.selectedItems
          |> List.map(item =>
               <div key=item> {ReasonReact.string(item)} </div>
             )
          |> Array.of_list
          |> ReasonReact.array
        }
      </h1>
      <select
        name="Tasks"
        value={self.state.selectedItems}
        multiple=true
        onChange={
          event =>
            self.send(SelectItem(ReactEvent.Form.target(event)##value))
        }>
        {
          self.state.items
          |> List.map(item =>
               <option value=item key=item>
                 {ReasonReact.string(item)}
               </option>
             )
          |> Array.of_list
          |> ReasonReact.array
        }
      </select>
    </div>,
};

@wfaler
Copy link

wfaler commented Aug 2, 2019

I'm encountering the same issue. Is there solution or workaround for this yet?

@esbullington
Copy link

esbullington commented Mar 26, 2020

I imagine an increasing number of people are going to run into this issue. There's an easy solution, but it'd be a breaking change to reason-react: making the value prop an unwrapped polymorphic variant with `Arr(array(string)) and `Str(string) constructors, instead of a string.

I'd be happy to submit a PR with that change, but is there any other non-breaking solution or workaround?

Edit: I think GADTs might also work if you didn't want the variants to be polymorphic for whatever reason, but would require the value type to be polymorphic which could be a headache and which you may be avoiding intentionally.

@peterpme peterpme added docs issues that are about providing better documentation question RFC labels Apr 22, 2020
@davesnx
Copy link
Member

davesnx commented Nov 23, 2022

That's a little harder in Reason than it might sound, we can't change the type definition of a function based on a value. Possible solutions like GADTs or moving this logic to the ppx are valid approaches to this, but unsure if the hassle makes sense here.

Smashing ideas here but, probably removing the multiple=true as a valid prop (add a warning when is used) and provide a multiselect with the array API?

It feels gross thought.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs issues that are about providing better documentation question RFC
Projects
None yet
Development

No branches or pull requests

5 participants