Skip to content

Feat/Picker selectionLimit prop #1270

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

Merged
merged 1 commit into from
Apr 27, 2021
Merged

Conversation

lidord-wix
Copy link
Contributor

Description

Add selectionLimit prop to the Picker component to support limiting the number of the selected items.
Jira 1702

Changelog

Add selectionLimit prop to the Picker component to support limiting the number of the selected items.

@lidord-wix lidord-wix requested a review from ethanshar April 21, 2021 12:39
@lidord-wix lidord-wix marked this pull request as draft April 22, 2021 13:22
@lidord-wix lidord-wix marked this pull request as ready for review April 22, 2021 13:43
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

Looks good!
wrote you one thing worth fixing

const accessibilityProps = {
accessibilityState: isSelected ? {selected: true} : undefined,
accessibilityHint: 'Double click to select this suggestion',
...Modifiers.extractAccessibilityProps(props)
};

const isItemDisabled = useMemo(() => {
return disabled || (!isSelected && context.selectionLimit && context.selectionLimit === selectedCounter);
}, [selectedCounter]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Notice you use a simple variable as a dependency in this useMemo.
Each time the component will render, selectedCounter will be recreated and will invoke the useMemo
Overall it's not that dramatic in this case, but better do it right.

I'd move selectedCounter into the useMemo method and include in the dependencies array values from props/context/state.

@ethanshar ethanshar added the Important for Next Release PR that must be included in the release version label Apr 27, 2021
@ethanshar ethanshar merged commit 8505bbf into master Apr 27, 2021
@lidord-wix lidord-wix deleted the feat/Picker_selectionLimit branch May 31, 2021 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants