Skip to content
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

fix(Dropdown): isEqual over shallowEqual for props #2252

Merged

Conversation

chrisjbrown
Copy link
Contributor

@chrisjbrown chrisjbrown commented Oct 25, 2017

fixes #2243

Component on componentWillReceiveProps will always run setSelectedIndex because shallowEqual on two objects internally runs object.is(a, b) to compare objects which will always be false because nextProps.options and this.props.options are not the same object

@layershifter layershifter changed the title fix(Dropdown): isequal over shallowequal for props fix(Dropdown): isEqual over shallowequal for props Oct 25, 2017
@layershifter layershifter changed the title fix(Dropdown): isEqual over shallowequal for props fix(Dropdown): isEqual over shallowEqual for props Oct 25, 2017
@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@de69da9). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2252   +/-   ##
=========================================
  Coverage          ?   99.73%           
=========================================
  Files             ?      152           
  Lines             ?     2652           
  Branches          ?        0           
=========================================
  Hits              ?     2645           
  Misses            ?        7           
  Partials          ?        0
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de69da9...5906fab. Read the comment docs.

@levithomason
Copy link
Member

Let's get a test here that fails with the workflow reported in the issue. This way we don't hit the regression again.

isEqual was removed as part of the lodash optimizations as it is a very slow and heavy method. We can use it here if we must, but I'd like to ensure there is a test that catches the failing behavior.

@chrisjbrown
Copy link
Contributor Author

Added UT for when props are updated with options that are the same and when props are updated with new options.

@levithomason levithomason merged commit 50c29ad into Semantic-Org:master Nov 4, 2017
@levithomason
Copy link
Member

Awesome, thank you very much @chrisjbrown!

@levithomason
Copy link
Member

Released in [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dropdown: setState in onChange leads to mismatch of 'active' and 'selected' option
4 participants