Skip to content

Conversation

@hyx131
Copy link
Contributor

@hyx131 hyx131 commented Aug 11, 2020

[STATUS: component to be finished by @DoParkEQ from here on]

This PR initially has the goal of implementing a multi-use-case <Filter /> component (see issue #96).

After discussions, goal for this PR has now changed to implementing a multi-select-group component, leaving out the outer wrapper (whether it'd be a button, or accordion, or none...) that opens up these select groups (see issue #98).

These select groups will internally manage any state change events, and will return the final state of those changes via onChange property, format of the result will depend on the select group type:

  • type 'radio' returns the selected value: (newVal) => newVal = selectedValue
  • type 'checkbox' returns 2 arrays:
    (newFilterOptions, selectedVals) => { newFilterOptions = [ { option1: true }, { option2: false } ] selectedVals = [ 'option1' ] }
  • type 'switch' returns 2 arrays (same arrays as type 'checkbox' returns)
  • type 'slider' returns 1 array: (newVal) => newVal = [min, max]

@hyx131 hyx131 changed the title [G2M]filter - filter component with checkbox select options [WIP]filter - filter component with checkbox select options Aug 12, 2020
@hyx131 hyx131 force-pushed the filter branch 3 times, most recently from de1b8ed to 34c2635 Compare August 13, 2020 04:03
@hyx131 hyx131 changed the title [WIP]filter - filter component with checkbox select options [G2M]filter - filter component with checkbox select options Aug 13, 2020
@hyx131 hyx131 changed the title [G2M]filter - filter component with checkbox select options [WIP]filter - filter component with checkbox select options Aug 21, 2020
@hyx131 hyx131 changed the title [WIP]filter - filter component with checkbox select options [G2M]filter - filter component with checkbox select options Aug 22, 2020
@parkdoeui
Copy link
Contributor

parkdoeui commented Aug 24, 2020

Nice work! I have a couple of discussion points regarding the recent change.

Component Hierarchy
It is my fault that I did not update the diagram after our discussion. I imagined the component hierarchy to be more like this:

Screen Shot 2020-08-24 at 2 01 21 PM

To me, the current SelectionGroup is the original Filter with a different name. In this diagram, all selection control types are wrapped under SelectionGroup. The difference with the current implementation would be:

  1. Grouping all four types of controls is covered under SelectionGroup. It means that we don't need to adopt <Type>Group for each control type.
  2. SelectionGroup itself is detached from other components such as Grid, FormControl, Typography, DynamicButton (just like CheckboxGroup)

Data type
Right now SelectionGroup accepts three data types: object, string, and number(integer and float). I noticed that if I want to switch control type from one to another, I also need to convert data types manually then feed them as options prop.

E.g. I need to manually change my data type from ['option 1', 'option 2', 'option 3' ] to [{option 1: false},{option 2: false},{option 3: false}] in order to change control type from radio to switch.

I feel that we should enforce a single data type (e.g. string) for all types of selection controls so that switching from one to another is more convenient from an end-user perspective.

<SelectionGroup
type='slider'
options={options}
optionsLabel='Categories'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just name it label?

<SwitchGroup filterVals={filterVals} switchOnChange={checkboxOnChange} direction={direction} />
)}
{type === 'slider' && (
<SliderGroup direction={direction} sliderStep={sliderStep} options={options} onChange={onChange} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, we don't have any plans to incorporate vertical sliders. It should be always horizontal regardless of direction.

const [filterVals, setFilterVals] = useState(options)
const [selectAll, setSelectAll] = useState(true)

const checkboxOnChange = (e) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name of this should be changed as it is used for both CheckboxGroup and SwitchGroup

import SliderGroup from './slider-group'


const useStyles = makeStyles(() => ({
Copy link
Contributor

@parkdoeui parkdoeui Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using theme.spacing prop for margins/paddings or have values that are a multiplication of 4.

@hyx131
Copy link
Contributor Author

hyx131 commented Aug 25, 2020

Nice work! I have a couple of discussion points regarding the recent change.

Component Hierarchy
It is my fault that I did not update the diagram after our discussion. I imagined the component hierarchy to be more like this:

Screen Shot 2020-08-24 at 2 01 21 PM

To me, the current SelectionGroup is the original Filter with a different name. In this diagram, all selection control types are wrapped under SelectionGroup. The difference with the current implementation would be:

  1. Grouping all four types of controls is covered under SelectionGroup. It means that we don't need to adopt <Type>Group for each control type.
  2. SelectionGroup itself is detached from other components such as Grid, FormControl, Typography, DynamicButton (just like CheckboxGroup)

Data type
Right now we options accept three data types: object, string, and number(integer and float). I noticed that if I want to switch control type from one to another, I also need to convert data types manually then feed them as options prop.

E.g. I need to manually change my data type from ['option 1', 'option 2', 'option 3' ] to [{option 1: false},{option 2: false},{option 3: false}] in order to change control type from radio to switch.

I feel that we should enforce a single data type (e.g. string) for all types of selection controls so that switching from one to another is more convenient from an end-user perspective.

Thanks, will review!

@hyx131 hyx131 changed the title [G2M]filter - filter component with checkbox select options [WIP]filter - filter component with checkbox select options Aug 25, 2020
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

Successfully merging this pull request may close these issues.

3 participants