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

feat(Select): Add the ability to have custom content in the Select Menu #3333

Merged
merged 3 commits into from Nov 21, 2019

Conversation

@tlabaj
Copy link
Contributor

tlabaj commented Nov 21, 2019

What: Closes #3193

Additional issues:

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 21, 2019

Codecov Report

Merging #3333 into master will decrease coverage by <.01%.
The diff coverage is 82.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3333      +/-   ##
==========================================
- Coverage   67.52%   67.52%   -0.01%     
==========================================
  Files         896      896              
  Lines       25100    25112      +12     
  Branches     2167     2173       +6     
==========================================
+ Hits        16950    16958       +8     
- Misses       7139     7141       +2     
- Partials     1011     1013       +2
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.28% <ø> (ø) ⬆️
#patternfly4 65.01% <82.14%> (ø) ⬆️
Impacted Files Coverage Δ
...-4/react-core/src/components/Select/SelectMenu.tsx 82.08% <75%> (-0.17%) ⬇️
...nfly-4/react-core/src/components/Select/Select.tsx 64.28% <87.5%> (+0.41%) ⬆️
...y-react-extensions/src/components/Select/Select.js 19.61% <0%> (-0.19%) ⬇️

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 7c9d4ae...3ad3d27. Read the comment docs.

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Nov 21, 2019

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

Copy link
Contributor

dlabaj left a comment

A few comments but looks good to me.

@@ -119,7 +121,8 @@ class Select extends React.Component<SelectProps & InjectedOuiaProps, SelectStat
onClear: (_e: React.MouseEvent) => undefined as void,
onCreateOption: (_newOptionValue: string) => undefined as void,
toggleIcon: null as React.ReactElement,
onFilter: null
onFilter: null,
customContent: null as React.ReactNode

This comment has been minimized.

Copy link
@dlabaj

dlabaj Nov 21, 2019

Contributor

This needs to be defined as ReactNode?

This comment has been minimized.

Copy link
@tlabaj

tlabaj Nov 21, 2019

Author Contributor

I was just following what was done for toggleIcon. I can change it.

This comment has been minimized.

Copy link
@dlabaj

dlabaj Nov 21, 2019

Contributor

I think if we are going to change it would should have it as a coding standard. It's fine to leave it ... however we might want to spend some time looking into our recommended coding standards so we are coding consistently through out the project. Maybe create an issue (chore) in our backlog to update coding standards for typescript and fix the code so we are consistent.

};

extendChildren() {
const { children, isGrouped } = this.props;
const childrenArray = children as React.ReactElement[];

This comment has been minimized.

Copy link
@dlabaj

dlabaj Nov 21, 2019

Contributor

Can you do this instead const childrenArray: React.ReactElement[] = children as React.ReactElement[]; ... so it's not implicit? Or will this not work with here?

Copy link
Contributor

mcoker left a comment

LGTM 👍

@tlabaj tlabaj dismissed stale reviews from mcoker, dlabaj, and kmcfaul via 3ad3d27 Nov 21, 2019
@tlabaj tlabaj force-pushed the tlabaj:select_basic branch from 032063d to 3ad3d27 Nov 21, 2019
@dlabaj
dlabaj approved these changes Nov 21, 2019
@dlabaj dlabaj merged commit 7f3b550 into patternfly:master Nov 21, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.