Skip to content

4259 Fix option not being rebuild after isOptionDisabled prop changes #4349

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

Conversation

manvydasu
Copy link
Contributor

#4259
This makes sure that options are being rebuilt whenever isOptionDisabled prop changes. The prop itself was not included into memoized option rebuild condition

@changeset-bot
Copy link

changeset-bot bot commented Dec 27, 2020

⚠️ No Changeset found

Latest commit: 0fc0401

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 27, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0fc0401:

Sandbox Source
react-codesandboxer-example Configuration

@manvydasu manvydasu force-pushed the fix/4259-fix-options-not-being-rebuild-when-isOptionDisabled-changes branch from b2b65b6 to cc0596b Compare December 27, 2020 19:37
@manvydasu manvydasu marked this pull request as ready for review December 28, 2020 13:01
@Methuselah96 Methuselah96 added this to the 3.2 milestone Jan 2, 2021
@Methuselah96 Methuselah96 linked an issue Jan 2, 2021 that may be closed by this pull request
@@ -378,7 +379,8 @@ export default class Select extends Component<Props, State> {

return isEqual(newSelectValue, lastSelectValue)
&& isEqual(newProps.inputValue, lastProps.inputValue)
&& isEqual(newProps.options, lastProps.options);
&& isEqual(newProps.options, lastProps.options)
&& newProps.isOptionDisabled === lastProps.isOptionDisabled;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to add functions to the memoization we should also include isOptionSelected, getOptionLabel, getOptionValue, filterOption, etc.

Copy link
Contributor Author

@manvydasu manvydasu Jan 16, 2021

Choose a reason for hiding this comment

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

I've added these and some other comparisons which are needed for rebuilding menu options. Do you expect to have tests for each of these? I've added one for the original issue.

@Methuselah96 Methuselah96 removed this from the 3.2 milestone Jan 7, 2021
@manvydasu manvydasu force-pushed the fix/4259-fix-options-not-being-rebuild-when-isOptionDisabled-changes branch from 63d053c to 2818847 Compare January 16, 2021 11:59
@Methuselah96
Copy link
Collaborator

@manvydasu Thanks for your work on this PR! We decided to actually take a different option and remove the memoization altogether (#4388) since we don't think it's realistic to maintain the memoization as it currently stands. We really appreciate your work on this and hope this solution works for you!

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.

isOptionDisabled does not update disabled items between renders
2 participants