-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix(FileUploaderDropContainer): fix NPE selecting file #4936
fix(FileUploaderDropContainer): fix NPE selecting file #4936
Conversation
5ead3ae
to
b66846a
Compare
Deploy preview for the-carbon-components ready! Built with commit 89c7a24 https://deploy-preview-4936--the-carbon-components.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 89c7a24 |
Deploy preview for carbon-components-react failed. Built with commit 89c7a24 https://app.netlify.com/sites/carbon-components-react/deploys/5e0146448ea6e30008190368 |
Deploy preview for the-carbon-components ready! Built with commit 87c3cf8 https://deploy-preview-4936--the-carbon-components.netlify.com |
Deploy preview for carbon-elements failed. Built with commit 87c3cf8 https://app.netlify.com/sites/carbon-elements/deploys/5e1d03182318c30008b50214 |
Deploy preview for carbon-components-react failed. Built with commit 87c3cf8 https://app.netlify.com/sites/carbon-components-react/deploys/5e1d031898d15f00081f45f8 |
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.
Just some style comments, otherwise seems great!
packages/react/src/components/FileUploader/FileUploader-test.js
Outdated
Show resolved
Hide resolved
const evt = { target: { files: mockFiles } }; | ||
input.simulate('change', evt); | ||
expect(onAddFiles).toHaveBeenCalledTimes(1); | ||
expect(onAddFiles.mock.calls[0][0].target.files).toEqual([ |
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.
Maybe expect(onAddFiles).toHaveBeenCalledWith
might be relevant here? Something like:
expect(onAddFiles).toHaveBeenCalledWith(expect.objectContaining({
target: {
files: [fileFoo, fileBar],
},
}));
packages/react/src/components/FileUploader/FileUploader-test.js
Outdated
Show resolved
Hide resolved
Should we remove the |
The story topic sounds orthogonal to this PR. |
Co-Authored-By: Josh Black <[email protected]>
just for context, during previous reviews it was decided that the story would be static and the full application example would be available in codesandbox |
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 to me
Updating the review status as all comments have been addressed.
@@ -255,7 +255,13 @@ describe('FileUploader', () => { | |||
}); | |||
|
|||
describe('FileUploaderDropContainer', () => { | |||
const dropContainer = <FileUploaderDropContainer className="extra-class" />; | |||
const onAddFiles = jest.fn(); | |||
const dropContainer = ( |
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.
We should move blocks where we create components into beforeEach
or a similar generator function to prevent test flakiness
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.
@joshblack Agreed on beforAll()
/beforeEach()
making tests more stable, but it's orthogonal to this PR. Do you want to enter an issue for that?
@joshblack is the test change request enough to warrant a separate issue? Or do you want it resolved in this PR before merging |
Let's address it before merging while we're in this part of the codebase and the change will be pretty quick 👍 |
@joshblack is right that there is a change to test setup code, but the discussion of change in style of test setup code is orthogonal. I want the works not distracted by such kind of change requests. |
If it helps, was only thinking the change would involve: describe('FileUploaderDropContainer', () => {
let onAddFiles;
let dropContainer;
let mountWrapper;
beforeEach(() => {
onAddFiles = jest.fn();
dropContainer = (
<FileUploaderDropContainer
className="extra-class"
onAddFiles={onAddFiles}
/>
);
mountWrapper = mount(dropContainer);
}); We should definitely get this update in there while we're working on this part of the codebase given how minimal the change is. |
OK feel free to push a patch like that. Even though the change is minimal, I see it's distracting. |
…bon-components into fileuploader-select-file-link
Can anyone help me with which release this might have gone into. |
I see the nightly release builds are failing. https://github.com/carbon-design-system/carbon/commit/d2619067329ea484d0d42442dab9638798f917dd/checks?check_suite_id=409159394 |
Hi @MaheshIBM! 👋 This was included in our v10.9.0 release, can see the commit over in: https://github.com/carbon-design-system/carbon/releases/tag/v10.9.0 You can see our release schedule over at: https://github.com/carbon-design-system/carbon/wiki/Release-radar Hope this helps! |
@joshblack I see the commit was done after the v10.9.0 release which happened on Dec 19 The code in the tag does not seem to have the fix. As per my understanding, I think this fix might go on 27th Jan 2020 as per screen shot below. If I compare the tag with master, I can see the commit showing up in the compare which means its not in the tag. |
@MaheshIBM good catch! Sorry about my mistake up above, not sure where I was finding this commit now. But yes, I believe it'll be in our next patch! |
Fixes #4899.
Changelog
Changed
<FileUploaderDropContainer>
.Testing / Reviewing
Testing should make sure file uploader is not broken.