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(ImageStream): Autocomplete ISTag dropdown #4714
Feat(ImageStream): Autocomplete ISTag dropdown #4714
Conversation
8289359
to
b6d2edb
Compare
b6d2edb
to
b1eef13
Compare
@abhinandan13jan I have verified locally, works as expected |
/kind feature |
@@ -44,6 +44,7 @@ export interface DropdownFieldProps extends FieldProps { | |||
title?: React.ReactNode; | |||
fullWidth?: boolean; | |||
disabled?: boolean; | |||
autoComplete?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several instances in the console where an autocompleteFilter
other than fuzzy
is being used. I suggest we keep the DropdownField
generic and accept an auto complete filter function.
autoComplete?: boolean; | |
autocompleteFilter?: (text: string, item: object, key: string) => boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -28,6 +29,7 @@ const DropdownField: React.FC<DropdownFieldProps> = ({ label, helpText, required | |||
<Dropdown | |||
{...props} | |||
id={fieldId} | |||
autocompleteFilter={props.autoComplete ? fuzzy : undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -1,5 +1,6 @@ | |||
import * as React from 'react'; | |||
import cx from 'classnames'; | |||
import * as fuzzy from 'fuzzysearch'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -111,6 +111,7 @@ const ImageStreamTagDropdown: React.FC = () => { | |||
label="Tag" | |||
items={imageStreamTagList} | |||
key={imageStream.image} | |||
autoComplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import fuzzy
then:
autoComplete | |
autocompleteFilter={fuzzy} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
b1eef13
to
b3f22f3
Compare
5bc72a3
to
eae402d
Compare
const deployImagePageProps: DeployImagePageProps = { | ||
history: null, | ||
location: { | ||
pathname: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
search: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
state: null, | ||
hash: null, | ||
}, | ||
match: { | ||
isExact: true, | ||
path: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
url: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
params: { | ||
ns: 'my-project', | ||
}, | ||
}, | ||
}; | ||
const deployImagePageWrapper: ReactWrapper = mount( | ||
<DeployImagePage {...deployImagePageProps} />, | ||
{ | ||
wrappingComponent: ({ children }) => <Provider store={store}>{children}</Provider>, | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should put this setup in beforeAll()
block and declare the variables in describe block
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props initialization is not working when put in beforeEach/beforeAll? what is wrong with the existing scoping?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the setup for unit-tests should be done in either beforeAll
or beforeEach
depending on the usage. it works when put in beforeAll
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated...
const deployImageProps: DeployImageProps = { | ||
projects: { | ||
data: [], | ||
loaded: false, | ||
}, | ||
namespace: 'my-project', | ||
}; | ||
|
||
const deployImageWrapper: ReactWrapper = mount(<DeployImage {...deployImageProps} />, { | ||
wrappingComponent: ({ children }) => <Provider store={store}>{children}</Provider>, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above
eae402d
to
5f021a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/test images |
path: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
url: 'deploy-image/ns/openshift?preselected-ns=openshift', | ||
params: { | ||
ns: 'my-project', | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can use same namespace in params and url/path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
5f021a3
to
e377734
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinandan13jan, christianvogt, invincibleJai, sahil143 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest Please review the full test history for this PR and help us cut down flakes. |
Fixes:
Addresses https://issues.redhat.com/browse/ODC-2430
Analysis / Root cause:
TypeAhead missing in imagestreamTag dropdown
Solution Description:
TypeAhead(autocompleteFilter) added to imagestreamTagdropdown at the child Dropdown Component
Screenshot
Test coverage
Added test Suite for Deploy image. Added test for DropdownField
Browser conformation
Chrome 73