cozy / cozy-ui

React components and CSS styles for Cozy apps
https://cozy.github.io/cozy-ui/react/
MIT License
48 stars 37 forks source link

SelectBox : Can't use SelectBox's reactSelectControl inside a render method #489

Open drazik opened 6 years ago

drazik commented 6 years ago

I am trying to use <SelectBox /> on the date filter component in Banks. For that, I need to use a custom Control component. I looked at the example on the styleguide and successfully provided a custom Control component to my <SelectBox />. Still, I need the content of this component to be dynamic, but doing the following doesn't work :

import React, { Component } from 'react'
import SelectBox, { reactSelectControl } from 'cozy-ui/react/SelectBox'

class Test extends Component {
  options = [
    { label: 'red', value: 'red' },
    { label: 'green', value: 'green' },
    { label: 'blue', value: 'blue' }
  ]

  state = {
    selected: this.options[0]
  }

  render() {
    return (
      <SelectBox
        defaultValue={this.options[0]}
        options={this.options}
        components={{
          Control: reactSelectControl(
            <button>{this.state.selected.label}</button>
          )
        }}
        onChange={selected => this.setState({ selected })}
      />
    )
  }
}

With this code, when I click in the control button, nothing happens. It doesn't even receive the focus.

If I externalize it, and make it static, it works :

import React, { Component } from 'react'
import SelectBox, { reactSelectControl } from 'cozy-ui/react/SelectBox'

const MyControl = reactSelectControl(<button>Click me</button>)

class Test extends Component {
  options = [
    { label: 'red', value: 'red' },
    { label: 'green', value: 'green' },
    { label: 'blue', value: 'blue' }
  ]

  state = {
    selected: this.options[0]
  }

  render() {
    return (
      <SelectBox
        defaultValue={this.options[0]}
        options={this.options}
        components={{
          Control: MyControl
        }}
        onChange={selected => this.setState({ selected })}
      />
    )
  }
}

Yet reactSelectControl is a simple HOC, so I don't understand what's going on here.

ptbrowne commented 6 years ago

I guess you cannot change the custom components of a ReactSelect during its lifetime. You should not changed the custom renderer (here MyControl) but instead rely on the fact that it gets passed the props of the select.

This example works in the styleguidist :

const options = [
  { value: 'chocolate', label: 'Chocolate' },
  { value: 'strawberry', label: 'Strawberry' },
  { value: 'vanilla', label: 'Vanilla' }
];

initialState = { label: 'toto' }

setTimeout(() => {
  setState({ label: 'hey' })
}, 1000);

<div>
  <SelectBox label={state.label} components={{ Control: props => <div>{props.selectProps.label}</div> }} options={options} />
</div>
drazik commented 6 years ago

Yay I didn't understand that, thank you ! I will try it and close the issue if it works for me.

ptbrowne commented 6 years ago

We should maybe make this clearer in the documentation before closing ;)

ptbrowne commented 6 years ago

IMHO, it is our API that is confusing at this point since it seems we are passing an instance of a component to SelectBox whereas we are in fact passing a Component : reactSelectControl is doing the change for us.

https://github.com/cozy/cozy-ui/blob/ef65f9bee24b8842c2437089341dc2f17ae4b338/react/SelectBox/index.jsx#L51-L56

If we passed a Component, I feel that it would be easier to presume that it cannot be changed it during the SelectBox lifetime.

<SelectBox components={ Control: MyControl } /> vs <SelectBox components={ Control: reactSelectControl(<MyControl />) } />

With the reactSelectControl API, it feels like we are dealing with a renderMethod and this makes the assumption that it can be changed during the lifetime plausible.

I think the reactSelectControl should not change an instance of a component to a Component to say closer to the original SelectBox API and not confusing. In the piece of a code above, I think CustomControl should be changed to <CustomControl /> so that instead of passing an instance, we would pass a Component.

What do you think @y-lohse ?

y-lohse commented 6 years ago

I think CustomControl should be changed to so that instead of passing an instance, we would pass a Component.

Yep ok, that sounds like it would be less confusing 👍

ptbrowne commented 6 years ago

@y-lohse Would be open to take this issue ?

y-lohse commented 6 years ago

I'll add it to my todo list, but I'm not sure when I'll be abble to work on this. If someone needs this done urgently, let me know!