vhx / quartz-react

Quartz components using React
2 stars 1 forks source link

Feature/select #11

Closed sebastiansandqvist closed 7 years ago

sebastiansandqvist commented 7 years ago

This is not ready to be merged. Publishing here a bit early so that I can get some feedback before going on to implement the <MediaSelect>.

Example code to demonstrate the API (basically demo/src/index.jsx) ```jsx const selectOpts = [ { label: 'Option #1', uniqueId: 'option-id-1' }, { label: 'Option #2', uniqueId: 'option-id-2' }, { label: 'Option #3', uniqueId: 'option-id-3' }, ]; const selectOptsWithDescription = [ { description: 'Hello World', label: 'Option #1', uniqueId: 'option-id-1' }, { description: 'Hello Quartz', label: 'Option #2', uniqueId: 'option-id-2' }, { description: 'Hello everyone', label: 'Option #3', uniqueId: 'option-id-3' }, ]; } /> {}} /> ); } } ```
sebastiansandqvist commented 7 years ago

Ready for review!

There are many places where this component gets complex internally. I would like to make it as easy to understand and use as possible, so if there's anything I can do to help with that then please let me know.

On that note, I'd go in this order to get an understanding of how it works:

  1. npm start and look at the browser console when using the <select>s
  2. See the usage in demo/src/index.jsx
  3. See the smart component in demo/src/demo-select.jsx
  4. See the implementation in components/Select/standard/Select.jsx.

"processing" state:

I initially wanted to be able to just have something like this for option state (as we discussed):

this.state = {
  optionState: {
    option1: 'checked',
    option2: 'unchecked',
    option3: 'unchecked',
    option4: 'processing',
  },
  ...
}

There was a lot of added complexity involved with doing it that way, so I opted for the way quartz currently handles processing state, which is to pass an array of processing option ids as a prop.

So currently it works like this:

this.state = {
  selectedOptions: {
    option1: true
    option2: false
    option3: false
    option4: false
  },
  processingOptions: [ 'option4' ],
  ...
}

The stateful component might look like this:

function removeFromArray(array, item) {
  const index = array.indexOf(item);
  if (index !== -1) { array.splice(index, 1); }
  return array;
}

class ProcessingSelect extends Component {
  constructor() {
    super();
    this.state = {
      isOpen: false,
      selectedOptions: {},
      selectedLabel: '',
      processingOptions: [],
    };
    this.setOpen = this.setOpen.bind(this);
    this.handleChange = this.handleChange.bind(this);
  }

  setOpen(isOpen) {
    this.setState({ isOpen });
  }

  handleChange(selectedOptions, selectedLabel, item) {
    const { processingOptions } = this.state;
    this.setState({ processingOptions: processingOptions.concat(item.uniqueId) });
    setTimeout(() => {
      this.setState({
        selectedOptions,
        selectedLabel,
        processingOptions: removeFromArray(processingOptions, item.uniqueId),
      });
    }, 500);
  }

  render() {
    return (
      <Select
        {...this.props}
        isOpen={this.state.isOpen}
        selectedOptions={this.state.selectedOptions}
        onSelectionToggle={this.handleChange}
        onOpenToggle={this.setOpen}
        triggerLabel={this.state.selectedLabel}
      />
    );
  }
}

// used like this:
<ProcessingSelect options={[/* some options go here */]} />

Initially I didn't like this way of handling "processing" state as much, but it has a few benefits.

  1. No string constants. You can do if (this.state.selectedOptions[foo]) { ... } instead of if (this.state.optionState[foo] === 'checked') { ... }
  2. Since most of the time you won't be using this option, the default api is simpler.
  3. Simpler implementation internally (easier to refactor)

(If this isn't ok, I can go back to the first way.)

Still left to do:

  1. ~Add tests~
  2. Maybe implement action and onAction (although I do not think this currently works, so I am not sure if it is needed). This image is a component that uses those props, and the action does not show up at all. screen shot 2017-06-19 at 4 15 20 pm
  3. Maybe add custom dropdown menu vertical offsets. Do you think this would be needed? I didn't see them in use in Crystal (but didn't look too hard).
  4. Add :hover state to standard dropdowns. I wanted to implement this in quartz css, but if that is not possible for now then I can use onMouseOver and onMouseOut to do the same thing.