-
Notifications
You must be signed in to change notification settings - Fork 345
feat(content-uploader): add drop support for modernized uploads manager #4674
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
base: master
Are you sure you want to change the base?
Changes from all commits
1ee697e
7df0d45
f8fb79c
7245efa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| import * as React from 'react'; | ||
| import omit from 'lodash/omit'; | ||
|
|
||
| import makeDroppable from '../common/droppable'; | ||
| import type { DOMStringList } from '../../common/types/core'; | ||
|
|
||
| export interface ModernizedUploadsManagerDropZoneProps extends React.HTMLAttributes<HTMLDivElement> { | ||
| addDataTransferItemsToUploadQueue: (droppedItems: DataTransfer) => void; | ||
| allowedTypes: Array<string>; | ||
| children: React.ReactNode; | ||
| className: string; | ||
| isDropEnabled?: boolean; | ||
| } | ||
|
|
||
| interface DroppableStateProps { | ||
| canDrop?: boolean; | ||
| isDragging?: boolean; | ||
| isOver?: boolean; | ||
| } | ||
|
|
||
| const dropDefinition = { | ||
| dropValidator: ( | ||
| { allowedTypes, isDropEnabled = true }: ModernizedUploadsManagerDropZoneProps, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry should dropenabled be true or false? i saw this f8fb79c#diff-71da4c8322f44025d67f136a0fd91e42a42d269dfc74654fa335bc7d24cc5c88R175-R177
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be false at the |
||
| { types }: { types: Array<string> | DOMStringList }, | ||
| ) => { | ||
| if (!isDropEnabled) { | ||
| return false; | ||
| } | ||
|
|
||
| if (types instanceof Array) { | ||
| return Array.from(types).some(type => allowedTypes.indexOf(type) > -1); | ||
| } | ||
|
|
||
| const allowedList = allowedTypes.filter(allowed => types.contains(allowed)); | ||
| return allowedList.length > 0; | ||
| }, | ||
|
|
||
| onDrop: (event: DragEvent, { addDataTransferItemsToUploadQueue }: ModernizedUploadsManagerDropZoneProps) => { | ||
| const { dataTransfer } = event; | ||
|
|
||
| if (!dataTransfer) { | ||
| return; | ||
| } | ||
|
|
||
| addDataTransferItemsToUploadQueue(dataTransfer); | ||
| }, | ||
| } as const; | ||
|
|
||
| const ModernizedUploadsManagerDropZoneComponent = React.forwardRef< | ||
| HTMLDivElement, | ||
| ModernizedUploadsManagerDropZoneProps & DroppableStateProps | ||
| >((props, ref) => { | ||
| const { children } = props; | ||
| const htmlProps: React.HTMLAttributes<HTMLDivElement> = omit(props, [ | ||
| 'addDataTransferItemsToUploadQueue', | ||
| 'allowedTypes', | ||
| 'canDrop', | ||
| 'children', | ||
| 'isDragging', | ||
| 'isDropEnabled', | ||
| 'isOver', | ||
| ]); | ||
|
|
||
| return ( | ||
| <div ref={ref} {...htmlProps}> | ||
| {children} | ||
| </div> | ||
| ); | ||
| }); | ||
|
|
||
| const ModernizedUploadsManagerDropZone = makeDroppable(dropDefinition)(ModernizedUploadsManagerDropZoneComponent); | ||
|
|
||
| export default ModernizedUploadsManagerDropZone; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,10 +3,12 @@ import { shallow } from 'enzyme'; | |
| import { UploadsManager as UploadsManagerBP } from '@box/uploads-manager'; | ||
| import * as UploaderUtils from '../../../utils/uploads'; | ||
| import Browser from '../../../utils/Browser'; | ||
| import { fireEvent, render, screen } from '../../../test-utils/testing-library'; | ||
| import { ContentUploaderComponent, CHUNKED_UPLOAD_MIN_SIZE_BYTES } from '../ContentUploader'; | ||
| import Footer from '../Footer'; | ||
| import UploadsManager from '../UploadsManager'; | ||
| import DroppableContent from '../DroppableContent'; | ||
| import ModernizedUploadsManagerDropZone from '../ModernizedUploadsManagerDropZone'; | ||
| import { | ||
| ERROR_CODE_ITEM_NAME_IN_USE, | ||
| STATUS_PENDING, | ||
|
|
@@ -159,6 +161,33 @@ describe('elements/content-uploader/ContentUploader', () => { | |
| expect(instance.itemIdsRef.current).toEqual(expected); | ||
| }); | ||
|
|
||
| test('should add rootFolderId to raw file upload options for the modernized uploads manager', () => { | ||
| const files = [{ name: 'yoyo', size: 1000 }]; | ||
| const wrapper = getWrapper({ | ||
| enableModernizedUploads: true, | ||
| rootFolderId: '12345', | ||
| useUploadsManager: true, | ||
| }); | ||
| wrapper.instance().upload = jest.fn(); | ||
|
|
||
| wrapper.setProps({ files }); | ||
|
|
||
| expect(wrapper.state().items[0].options.folderId).toBe('12345'); | ||
| }); | ||
|
|
||
| test('should not add rootFolderId to raw file upload options outside the modernized uploads manager', () => { | ||
| const files = [{ name: 'yoyo', size: 1000 }]; | ||
| const wrapper = getWrapper({ | ||
| rootFolderId: '12345', | ||
| useUploadsManager: true, | ||
| }); | ||
| wrapper.instance().upload = jest.fn(); | ||
|
|
||
| wrapper.setProps({ files }); | ||
|
|
||
| expect(wrapper.state().items[0].options.folderId).toBeUndefined(); | ||
| }); | ||
|
|
||
| test('should handle accepting package "files" separate from folders', () => { | ||
| const mockFile = { name: 'hi' }; | ||
| Browser.isSafari = jest.fn(() => true); | ||
|
|
@@ -964,6 +993,127 @@ describe('elements/content-uploader/ContentUploader', () => { | |
| }); | ||
| }); | ||
|
|
||
| describe('ModernizedUploadsManagerDropZone', () => { | ||
| const renderModernizedUploadsManagerDropZone = props => | ||
| render( | ||
| <ModernizedUploadsManagerDropZone | ||
| addDataTransferItemsToUploadQueue={jest.fn()} | ||
| allowedTypes={['Files']} | ||
| className="bcu-modernized-panel" | ||
| data-testid="bcu-modernized-panel" | ||
| {...props} | ||
| > | ||
| <div /> | ||
| </ModernizedUploadsManagerDropZone>, | ||
| ); | ||
|
|
||
| test('prevents browser navigation and queues files dropped on the modernized panel', () => { | ||
| const addDataTransferItemsToUploadQueue = jest.fn(); | ||
| const dataTransfer = { | ||
| items: ['file-item'], | ||
| types: ['Files'], | ||
| }; | ||
| const dragEnterEvent = { | ||
| dataTransfer, | ||
| preventDefault: jest.fn(), | ||
| }; | ||
| const dropEvent = { | ||
| dataTransfer, | ||
| preventDefault: jest.fn(), | ||
| }; | ||
| const wrapper = shallow( | ||
| <ModernizedUploadsManagerDropZone | ||
| addDataTransferItemsToUploadQueue={addDataTransferItemsToUploadQueue} | ||
| allowedTypes={['Files']} | ||
| className="bcu-modernized-panel" | ||
| > | ||
| <div /> | ||
| </ModernizedUploadsManagerDropZone>, | ||
| ); | ||
|
|
||
| wrapper.instance().handleDragEnter(dragEnterEvent); | ||
| wrapper.instance().handleDrop(dropEvent); | ||
|
|
||
| expect(dragEnterEvent.preventDefault).toHaveBeenCalled(); | ||
| expect(dropEvent.preventDefault).toHaveBeenCalled(); | ||
| expect(addDataTransferItemsToUploadQueue).toHaveBeenCalledWith(dataTransfer); | ||
| }); | ||
|
|
||
| test('prevents browser navigation without queueing when isDropEnabled is false', () => { | ||
| const addDataTransferItemsToUploadQueue = jest.fn(); | ||
| const dataTransfer = { | ||
| items: ['file-item'], | ||
| types: ['Files'], | ||
| }; | ||
| const dragEnterEvent = { | ||
| dataTransfer, | ||
| preventDefault: jest.fn(), | ||
| }; | ||
| const dropEvent = { | ||
| dataTransfer, | ||
| preventDefault: jest.fn(), | ||
| }; | ||
| const wrapper = shallow( | ||
| <ModernizedUploadsManagerDropZone | ||
| addDataTransferItemsToUploadQueue={addDataTransferItemsToUploadQueue} | ||
| allowedTypes={['Files']} | ||
| isDropEnabled={false} | ||
| className="bcu-modernized-panel" | ||
| > | ||
| <div /> | ||
| </ModernizedUploadsManagerDropZone>, | ||
| ); | ||
|
|
||
| wrapper.instance().handleDragEnter(dragEnterEvent); | ||
| wrapper.instance().handleDrop(dropEvent); | ||
|
|
||
| expect(dragEnterEvent.preventDefault).toHaveBeenCalled(); | ||
| expect(dropEvent.preventDefault).toHaveBeenCalled(); | ||
| expect(addDataTransferItemsToUploadQueue).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('does not forward droppable state props to the modernized panel DOM element', () => { | ||
| const consoleError = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
| const dataTransfer = { | ||
| items: ['file-item'], | ||
| types: ['Files'], | ||
| }; | ||
| let consoleErrors = ''; | ||
|
|
||
| try { | ||
| renderModernizedUploadsManagerDropZone(); | ||
|
|
||
| fireEvent.dragEnter(screen.getByTestId('bcu-modernized-panel'), { dataTransfer }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i know this test id was already in here but i have to ask, is it totally necessary to use it here? we always prefer to use the role based lookups
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. EUA’s dropzone component also use |
||
|
|
||
| consoleErrors = consoleError.mock.calls.flat().join('\n'); | ||
| } finally { | ||
| consoleError.mockRestore(); | ||
| } | ||
|
|
||
| expect(consoleErrors).not.toContain('isDragging'); | ||
| expect(consoleErrors).not.toContain('isOver'); | ||
| }); | ||
|
|
||
| test('does not queue files dropped on the modernized panel when isDropEnabled is false', () => { | ||
| const addDataTransferItemsToUploadQueue = jest.fn(); | ||
| const dataTransfer = { | ||
| items: ['file-item'], | ||
| types: ['Files'], | ||
| }; | ||
|
|
||
| renderModernizedUploadsManagerDropZone({ | ||
| addDataTransferItemsToUploadQueue, | ||
| isDropEnabled: false, | ||
| }); | ||
|
|
||
| const modernizedPanel = screen.getByTestId('bcu-modernized-panel'); | ||
| fireEvent.dragEnter(modernizedPanel, { dataTransfer }); | ||
| fireEvent.drop(modernizedPanel, { dataTransfer }); | ||
|
|
||
| expect(addDataTransferItemsToUploadQueue).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('render()', () => { | ||
| describe('enableModernizedUploads', () => { | ||
| test('should render legacy UploadsManager when enableModernizedUploads is false and useUploadsManager is true', () => { | ||
|
|
@@ -985,6 +1135,13 @@ describe('elements/content-uploader/ContentUploader', () => { | |
| expect(wrapper.find(UploadsManagerBP)).toHaveLength(1); | ||
| expect(wrapper.find(UploadsManager)).toHaveLength(0); | ||
| expect(wrapper.find(DroppableContent)).toHaveLength(0); | ||
| expect(wrapper.find(ModernizedUploadsManagerDropZone)).toHaveLength(1); | ||
| }); | ||
|
|
||
| test('should disable dropping on the modernized panel when modernized drop props are omitted', () => { | ||
| const wrapper = getWrapper({ enableModernizedUploads: true }); | ||
|
|
||
| expect(wrapper.find(ModernizedUploadsManagerDropZone).prop('isDropEnabled')).toBe(false); | ||
| }); | ||
|
|
||
| test('should render modernized UploadsManagerBP even when useUploadsManager is true', () => { | ||
|
|
||
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 know this isn't a part of your pr but one thing i've been looking for in recent PRs are unnecessary guards. is uploadAPIOptions ever falsy for a file? i see it's defined a few lines above
https://github.com/box/box-ui-elements/pull/4674/changes#diff-71da4c8322f44025d67f136a0fd91e42a42d269dfc74654fa335bc7d24cc5c88R664
and i see that function always returns something ie it should be truthy
box-ui-elements/src/utils/uploads.js
Lines 76 to 79 in f8fb79c
in other repos i know we have quality gates for cognitive load (ie extra or unused if conditions), but not so much in this one, however it might still be good to clean this up while you're in here. up to you though on how much creep you're ok with in this pr
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 would like to avoid changing it as part of this PR because I do not have enough context why this guard is there. If something breaks it could force us to revert the whole drop support as well, so I’d rather keep this PR scoped to the related changes.