-
Notifications
You must be signed in to change notification settings - Fork 43
Filter fix #237
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
Filter fix #237
Conversation
✅ Deploy Preview for quickstarts ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
package.json
Outdated
| "test:a11y": "patternfly-a11y --config patternfly-a11y.config" | ||
| }, | ||
| "dependencies": { | ||
| "@patternfly/react-core": "^4.276.8" |
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.
This should not be added here - the dependency is defined in the module/package.json - we can increase the version required there, though.
|
We are facing this issue in the Openshift console quick starts https://issues.redhat.com/browse/OCPBUGS-13359. When will this get in? |
|
hi - we requested changes on this PR - I haven't seen an update to this but we can certainly prioritize closing this and fixing this a different way. |
dlabaj
left a comment
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.
looks good other then the package.json that should be updated is in module
dlabaj
left a comment
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 the dependency @dgutride @radekkaluzik . Let me know if you see anything else.
dlabaj
left a comment
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 the dependency @dgutride @radekkaluzik . Let me know if you see anything else.
dlabaj
left a comment
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 the dependency @dgutride @radekkaluzik . Let me know if you see anything else.
dlabaj
left a comment
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 the dependency @dgutride @radekkaluzik . Let me know if you see anything else.
dlabaj
left a comment
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 the dependency @dgutride @radekkaluzik . Let me know if you see anything else.
| } | ||
| export const QuickStartCatalogFilterSearchWrapper: React.FC<QuickStartCatalogFilterSearchWrapperProps> = ({ | ||
| onSearchInputChange = () => {}, | ||
| onSearchInputChange = (val?:string) => {}, |
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.
Can't you create a function in open shift to handle the val being passed in like this? It takes a parameter of any.
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.
@radekkaluzik @dlabaj That's correct, this change isn't needed, it just changes the type of this implementation, but this is then ignored in the function itself.
If you want apply the right type you need to update the type definition of QuickStartCatalogFilterSearchWrapperProps above.
| }; | ||
|
|
||
| interface QuickStartCatalogFilterSearchWrapperProps { | ||
| onSearchInputChange: any; |
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.
You want change this here:
| onSearchInputChange: any; | |
| onSearchInputChange: (val: string); |
| } | ||
| export const QuickStartCatalogFilterSearchWrapper: React.FC<QuickStartCatalogFilterSearchWrapperProps> = ({ | ||
| onSearchInputChange = () => {}, | ||
| onSearchInputChange = (val?:string) => {}, |
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.
@radekkaluzik @dlabaj That's correct, this change isn't needed, it just changes the type of this implementation, but this is then ignored in the function itself.
If you want apply the right type you need to update the type definition of QuickStartCatalogFilterSearchWrapperProps above.
Reason for this fix: https://issues.redhat.com/browse/RHCLOUD-25108
There is a workaround in place: https://github.com/RedHatInsights/learning-resources/pull/12/files
There is a breaking change in the latest version of PatternFly (this is why I have added "@patternfly/react-core" to dependencies). SearchInput onChange method expects
eventas its first argument and then the actual string.