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(FileUpload): added event to onTextChange #8955
feat(FileUpload): added event to onTextChange #8955
Conversation
Preview: https://patternfly-react-pr-8955.surge.sh A11y report: https://patternfly-react-pr-8955-a11y.surge.sh |
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.
Nice, left a small comment below @wise-king-sullyman but I ran into some challenges testing with our FileUpload demos; looks like somewhere along the road to v5 our FileUpload examples broke.
Did some hunting and it seems like this bug may have been introduced with #7926 ... should we open an issue @tlabaj ?
@@ -114,8 +114,8 @@ export const FileUploadField: React.FunctionComponent<FileUploadFieldProps> = ({ | |||
|
|||
...props | |||
}: FileUploadFieldProps) => { | |||
const onTextAreaChange = (newValue: string) => { | |||
onTextChange?.(newValue); | |||
const onTextAreaChange = (newValue: string, event: React.ChangeEvent<HTMLTextAreaElement>) => { |
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.
If we're following the convention that event
is first for these functions, should the order of these parameters be swapped?
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.
Yeah I debated how I wanted to approach this. Since the onChange
of TextArea that this gets passed into is still on the todo list of the param adding epic I went with just leaving this internal function in the order that it's in, but I could update the order here and just swap them in the onChange
, either way works if you have a preference.
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.
Other than the issue noted above, looks good.
@jenny-s51 is the issue you're running into the following error when trying to upload something?
I'm getting that both in this PR's surge and the surge from the PR you linked where the dropzone was upgraded. In the React v5 surge things work fine for me, though.
@thatblindgeye you don't get the error on https://patternfly-react-v5.surge.sh/components/file-upload/simple-file-upload? |
@wise-king-sullyman apparently the bookmark I had was an |
Ok good, so it's not a regression in this PR then. I went to make an issue but it looks like #8969 will probably address it. |
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.
Since there's a followup for the bug above, ✅
Your changes have been released in:
Thanks for your contribution! 🎉 |
To set the proper value after PatternFly 5 added the event as first arg for these callback props. See patternfly/patternfly-react#8955 and patternfly/patternfly-react#8955 PR based on changes made by https://github.com/patternfly/pf-codemods/
To set the proper value after PatternFly 5 added the event as first arg for these callback props. See patternfly/patternfly-react#8955 and patternfly/patternfly-react#8955 Commit based on changes made by https://github.com/patternfly/pf-codemods/
To set the proper value after PatternFly 5 added the event as first arg for these callback props. See patternfly/patternfly-react#8955 and patternfly/patternfly-react#8955 Commit based on changes made by https://github.com/patternfly/pf-codemods/
What: Closes #8882
Additional issues: