Skip to content
This repository has been archived by the owner on Jul 28, 2020. It is now read-only.

ComboBox stateful with hooks #750 #793

Merged
merged 10 commits into from
Mar 4, 2019
Merged

ComboBox stateful with hooks #750 #793

merged 10 commits into from
Mar 4, 2019

Conversation

damian-rodriguez-sociomantic
Copy link
Contributor

PR for #750

@damian-rodriguez-sociomantic
Copy link
Contributor Author

Blocked till we update all components with useTheme custom Hook

conor-cafferkey-sociomantic
conor-cafferkey-sociomantic approved these changes Jan 30, 2019
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Boom 💥 Other than updating to use useTheme (which is blocked) this looks perfect to me!

One thing: maybe it’s worth generalizing the useSelectedOption custom hook and adding it to src/utils/customHooks.js. Only question is what to call it 🤔 usePropOverridesState?

daniel-martic-sociomantic
daniel-martic-sociomantic suggested changes Jan 31, 2019
View reviewed changes
Copy link
Contributor

@daniel-martic-sociomantic daniel-martic-sociomantic left a comment

Choose a reason for hiding this comment

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

Hell yeah! 🔥 I have only one tiny thing to add:

src/ComboBox/index.jsx Outdated Show resolved Hide resolved
conor-cafferkey-sociomantic
conor-cafferkey-sociomantic suggested changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Great stuff! Just a couple minor things I noticed:

The following functions could use useCallback with an empty array as second arg:

  • handleClick
  • handleMouseOutOption
  • handleBlur

src/ComboBox/index.jsx Outdated
value,
} = props;

const [ optionSelected, setOptionSelected ] = useSelection(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call it selection and setSelection instead? optionSelected implies a single option but it could be multiple

src/ComboBox/index.jsx Outdated
const { isMultiselect, onChange } = this.props;
const unprefixedId = removePrefix( optId, id );

let newSelection = !isReadOnly ? getOption(
Copy link
Contributor

Choose a reason for hiding this comment

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

Calling getOption here is redundant (I think) since we already have the id of the new option (unprefixedId)

src/ComboBox/index.jsx Outdated
value : undefined,
};

ComboBox.displayName = 'ComboBox';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ComboBox.displayName = 'ComboBox';
ComboBox.displayName = componentName;

src/ComboBox/index.jsx Outdated
[ props.id ],
);

const filteredOptions = searchValue && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use useMemo with [ flatOptions, searchValue ]

src/ComboBox/index.jsx

handleClickClose( { id } )
const handleClickClose = useCallback( ( { id : tagId } ) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Array of props to watch is missing from this useCallback

conor-cafferkey-sociomantic
conor-cafferkey-sociomantic suggested changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

💥

Just one thing:

src/ComboBox/index.jsx Outdated
const activeOption = useMemo(
() => ( ( searchValue && filteredOptions.length ) ?
filteredOptions[ 0 ].id : stateActiveOption ),
[ flatOptions, searchValue, stateActiveOption ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[ flatOptions, searchValue, stateActiveOption ],
[ filteredOptions, searchValue, stateActiveOption ],

@conor-cafferkey-sociomantic conor-cafferkey-sociomantic merged commit 8ffb3a4 into Milestone-Hooks! Mar 4, 2019
@conor-cafferkey-sociomantic conor-cafferkey-sociomantic deleted the Issue-750-ComboBoxWithHooks! branch March 4, 2019 15:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@daniel-martic-sociomantic daniel-martic-sociomantic daniel-martic-sociomantic approved these changes

@conor-cafferkey-sociomantic conor-cafferkey-sociomantic conor-cafferkey-sociomantic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@damian-rodriguez-sociomantic @conor-cafferkey-sociomantic @daniel-martic-sociomantic