Skip to content
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(file-uploader): add drop and drop file uploader #3873

Merged
merged 4 commits into from
Sep 13, 2019

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Aug 29, 2019

Closes #2288

refs #2848 #3016 #3175

This PR adds drag and drop support to the vanilla file uploader component using native HTML APIs for drag and drop events.

Changelog

New

  • file uploader drag and drop support
  • example application showing drag and drop file uploader workflow, error handling, etc

Changed

  • correct file upload status icons
  • match invalid file upload markup structure with loading and success upload structures
  • deprecate legacy styles

Testing / Reviewing

Ensure the new file uploader functions as expected and ensure the existing file loader does not have any regressions

@netlify
Copy link

netlify bot commented Aug 29, 2019

Deploy preview for the-carbon-components ready!

Built with commit 86eef5c

https://deploy-preview-3873--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Aug 29, 2019

Deploy preview for carbon-elements ready!

Built with commit 86eef5c

https://deploy-preview-3873--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Aug 29, 2019

Deploy preview for carbon-components-react ready!

Built with commit 86eef5c

https://deploy-preview-3873--carbon-components-react.netlify.com

@emyarod emyarod force-pushed the dnd-file-uploader-1 branch 4 times, most recently from 9469c00 to 3b17fbe Compare September 4, 2019 20:44
@jnm2377 jnm2377 requested a review from a team September 10, 2019 20:40
@ghost ghost requested review from jnm2377 and vpicone and removed request for a team September 10, 2019 20:40
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 - Thanks @emyarod!

Copy link
Contributor

@shixiedesign shixiedesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there! Just 1 comment:

  • Looks like the loader is still miss-aligned
    image

@emyarod
Copy link
Member Author

emyarod commented Sep 12, 2019

I think we will need to make changes to the inline loader to resolve this alignment issue. The bounding box of the loading svg is different from the icons I believe, will look into this further and open a separate ticket if needed

const isOfSelf = this.element.contains(evt.target);
// In IE11 `evt.dataTransfer.types` is a `DOMStringList` instead of an array
if (
Array.prototype.indexOf.call(evt.dataTransfer.types, 'Files') >= 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am unsure about what this first array actually is and what it's calling.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since evt.dataTransfer.types is a DOMStringList and not an array we cannot directly call the indexOf method on evt.dataTransfer.types. so we call (Function.prototype.call) Array.prototype.indexOf and pass in the DOMStringList as the list of function arguments

an alternative way to do this would be [...evt.dataTransfer.types].indexOf('Files') >= 0 && ...

does that help?

Copy link
Contributor

@jnm2377 jnm2377 Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok yes. That does help. In terms of when 'Files' would be present or not, what kind of situations would those be?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it depending on the kind of file? or just that it's a file of some sort?

Copy link
Member Author

@emyarod emyarod Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Files' will be added to the types array if the event is a drag event and it involves any file https://html.spec.whatwg.org/multipage/dnd.html#dom-datatransfer-types-dev

if (evt.type === 'dragover') {
evt.preventDefault();
const dropEffect = inArea ? 'copy' : 'none';
if (Array.isArray(evt.dataTransfer.types)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we checking if this is an array bc of your comment in line 194 (// In IE11 evt.dataTransfer.types is a DOMStringList instead of an array)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that's right

Copy link
Contributor

@shixiedesign shixiedesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like loader's offset is going to be handled in a different issue. This is good to merge. Great work! 🎉

@emyarod emyarod requested a review from a team as a code owner September 13, 2019 13:18
@ghost ghost requested a review from aledavila September 13, 2019 13:18
@emyarod
Copy link
Member Author

emyarod commented Sep 13, 2019

@shixiedesign I resolved the offset loading spinner in this issue with a temp fix, the main issues can be addressed in a separate ticket

@asudoh asudoh merged commit 0074e22 into carbon-design-system:master Sep 13, 2019
@emyarod emyarod deleted the dnd-file-uploader-1 branch September 16, 2019 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FileUploader] Drag and drop support in file uploader
4 participants