Skip to content
This repository was archived by the owner on Feb 7, 2022. It is now read-only.

properly check "search" property value #13

Merged
merged 2 commits into from
Sep 4, 2015

Conversation

rutsky
Copy link
Contributor

@rutsky rutsky commented Sep 4, 2015

Fixes react warning when passing "search={false}".

Fixes react warning when passing "search={false}".
@Lapple
Copy link
Owner

Lapple commented Sep 4, 2015

Thanks, but will the warning go away if you simply pass oneOfType array of func and boolean?

@rutsky
Copy link
Contributor Author

rutsky commented Sep 4, 2015

@Lapple, yes, but then it will be possible to pass true value, which isn't allowed (and looks like will lead to internal exception).

@Lapple
Copy link
Owner

Lapple commented Sep 4, 2015

While technically this is possible, I am not sure if that is a common pitfall. I agree with the PR subject that passing false should not trigger a warning that contradicts with the docs. I just think that having such an extensive error reporting costs too much for the value it brings. Do you agree?

@rutsky
Copy link
Contributor Author

rutsky commented Sep 4, 2015

I agree that there is a lot of code for a very small and uncommon bug.

I thought to write oneOfType(boolean, function) initially, but decided that this is technically not correct due to missing true value case.

If you want, I can push oneOfType solution.

@rutsky
Copy link
Contributor Author

rutsky commented Sep 4, 2015

I pushed in this branch incorrectly checked isRequired fix.

I checked that:

  • if property is omitted internal search box appears,
  • passing false disables internal search box,
  • passing string leads to React warning (followed by exception which is expected),
  • passing true leads to exception (which is bug, but it's not worth fixing).

Lapple added a commit that referenced this pull request Sep 4, 2015
properly check "search" property value
@Lapple Lapple merged commit 0e5c08a into Lapple:master Sep 4, 2015
@Lapple
Copy link
Owner

Lapple commented Sep 4, 2015

Perfect, thank you!

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

Successfully merging this pull request may close these issues.

2 participants