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(Select): set onFilter to null to run the default filter #3158

Merged
merged 1 commit into from Oct 22, 2019

Conversation

@boaz0
Copy link
Member

boaz0 commented Oct 17, 2019

What:

fixes #3167

Typeahead doesn't filter options correctly. For example, in the Typeahead select input example, clicking "Ala" will display all the results even though they don't contain this substring.

The problem is that in the defaultProps object the onFilter attribute is set to a noop function which makes the if(onFilter) block to execute and eventually sets options to the children.

This PR fixes it by setting onFilter to null which makes the default filter function in onChange to be executed.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Oct 17, 2019

PatternFly-React preview: https://patternfly-react-pr-3158.surge.sh

Copy link
Member

dlabrecq left a comment

Hi Boaz, I tested the typeahead select input example, via the preview link above, and I'm still able to see all options upon a click.

Would you please create an issue and attach a screen capture showing what you're trying to solve? That is, if it's any different than what I'm testing below.

chrome-capture (1)

@boaz0 boaz0 force-pushed the boaz0:fixes_select_typeahead branch from e1e7109 to 10ed080 Oct 18, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 18, 2019

Codecov Report

Merging #3158 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3158      +/-   ##
=========================================
+ Coverage   68.98%     69%   +0.01%     
=========================================
  Files         858     858              
  Lines       23627   23619       -8     
  Branches     1893    1889       -4     
=========================================
- Hits        16300   16299       -1     
+ Misses       6366    6359       -7     
  Partials      961     961
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 67.99% <ø> (+0.04%) ⬆️
Impacted Files Coverage Δ
...nfly-4/react-core/src/components/Select/Select.tsx 61.78% <ø> (+2.92%) ⬆️
...-4/react-core/src/components/withOuia/withOuia.tsx 76.92% <0%> (-1.65%) ⬇️
...ernfly-4/react-table/src/components/Table/Body.tsx 72.34% <0%> (-0.58%) ⬇️
...-4/react-table/src/components/Table/RowWrapper.tsx 77.55% <0%> (-0.45%) ⬇️
...rnfly-4/react-table/src/components/Table/Table.tsx 91.35% <0%> (ø) ⬆️
...y-3/patternfly-react/src/components/Table/Table.js 95.74% <0%> (ø) ⬆️
...omponents/Table/utils/decorators/cellHeightAuto.ts 100% <0%> (ø) ⬆️
...y-react-extensions/src/components/Select/Select.js 19.8% <0%> (+0.09%) ⬆️

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 3a965e4...58710af. Read the comment docs.

@boaz0 boaz0 force-pushed the boaz0:fixes_select_typeahead branch from 10ed080 to d3de5d5 Oct 18, 2019
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Oct 18, 2019

@dlabrecq good catch. I think I fixed it here too - I just passed this.onClick to the input box and that worked

@boaz0 boaz0 force-pushed the boaz0:fixes_select_typeahead branch from d3de5d5 to 58710af Oct 18, 2019
- set onFilter to null to run the default filter
- pass 'this.onClick' to inputs to avoid reseting options

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
@boaz0 boaz0 force-pushed the boaz0:fixes_select_typeahead branch from 58710af to 749c38d Oct 18, 2019
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Oct 18, 2019

@boaz . are you still having an issue? If so, can you open an issue for this. Thanks.

Copy link
Member

dlabrecq left a comment

Retested all select typahead examples and seems to be working. Just need to attach an issue

@tlabaj tlabaj requested a review from kmcfaul Oct 21, 2019
Copy link
Contributor

jenny-s51 left a comment

LGTM and works as expected

@kmcfaul kmcfaul merged commit ca4dff7 into patternfly:master Oct 22, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@boaz0

This comment has been minimized.

Copy link
Member Author

boaz0 commented Oct 22, 2019

Thank you all 🐱

@boaz0 boaz0 deleted the boaz0:fixes_select_typeahead branch Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.