FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(Select): set onFilter to null to run the default filter by boaz0 · Pull Request #3158 · patternfly/patternfly-react · GitHub

/ patternfly-react Public

fix(Select): set onFilter to null to run the default filter - #3158

Merged
kmcfaul merged 1 commit into
patternfly:masterfrom
boaz0:fixes_select_typeahead
Oct 22, 2019
ConversationCommits (1)ChecksFiles changed
Merged

fix(Select): set onFilter to null to run the default filter#3158
kmcfaul merged 1 commit into
patternfly:masterfrom
boaz0:fixes_select_typeahead

Conversation

boaz0 commented Oct 17, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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 commented Oct 17, 2019

Copy link
Copy Markdown
Collaborator

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

dlabrecq suggested changes Oct 18, 2019
View reviewed changes

dlabrecq left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread packages/patternfly-4/react-core/src/components/Select/Select.tsx Outdated

codecov-io commented Oct 18, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 commented Oct 18, 2019

Copy link
Copy Markdown
Member Author

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

fix(Select): display typeahead filtered options
749c38d 
- 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>

tlabaj commented Oct 18, 2019

Copy link
Copy Markdown
Contributor

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

dlabrecq reviewed Oct 18, 2019
View reviewed changes

dlabrecq left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

tlabaj requested a review from kmcfaul October 21, 2019 15:25
jenny-s51 approved these changes Oct 21, 2019
View reviewed changes

jenny-s51 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM and works as expected

kmcfaul merged commit ca4dff7 into patternfly:master Oct 22, 2019

boaz0 commented Oct 22, 2019

Copy link
Copy Markdown
Member Author

Thank you all 🐱

boaz0 deleted the fixes_select_typeahead branch October 22, 2019 22:00
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kmcfaul kmcfaul approved these changes
+2 more reviewers
dlabrecq dlabrecq approved these changes
jenny-s51 jenny-s51 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

kmcfaul

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Select: Typeahead (multi/single) doesn't filter options

7 participants

Back | FazBrowse Home | New Git URL