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

Rewrite dropzone initialization #1082

Merged
merged 1 commit into from Apr 26, 2017

Conversation

Projects
None yet
4 participants
@cproensa
Contributor

cproensa commented Apr 6, 2017

This replaces #1077 as a better approach.

Some changes has been done to Dropzone initialization:

  • Container form now does not need any special class.
  • Dropzone specific data attributes are now placed in the Dropzone div
    element, which is indicated as "dropzone" class.
  • Auto upload feature is now enabled by adding the additional
    "auto-dropzone" class to current "dropzone" div.

Dropzone initialization searchs for elements with class "dropzone", then
searchs for the container form and uses it as parameter for the object,
and register event listeners.

Fixes: #22140, #22673

Rewrite dropzone initialization
Some changes has been done to Dropzone initialization:
- Container form now does not need any special class.
- Dropzone specific data attributes are now placed in the Dropzone div
  element, which is indicated as "dropzone" class.
- Auto upload feature is now enabled by adding the additional
  "auto-dropzone" class to current "dropzone" div.

Dropzone initialization searchs for elements with class "dropzone", then
searchs for the container form and uses it as parameter for the object,
and register event listeners.

Fixes: #22140, #22673
@vboctor

Looks good. One minor comment.

enableDropzone( "auto-dropzone", true );
}
$( 'form .dropzone' ).each(function(){
var classPrefix = 'dropzone';

This comment has been minimized.

@vboctor

vboctor Apr 6, 2017

Member

Do we need this variable vs. just using the string directly? Why not have the class hard-coded in enableDropzone()?

@vboctor

vboctor Apr 6, 2017

Member

Do we need this variable vs. just using the string directly? Why not have the class hard-coded in enableDropzone()?

This comment has been minimized.

@cproensa

cproensa Apr 6, 2017

Contributor

I guess that enableDropzone( classPrefix, autoUpload ) was written with the idea to allow different class names to match the dropzone elements. For example, allowing more than one DZ, for different forms, in the same page.
However, i think that the library is internally dependant on elements with "dropzone" class. At least, i couldn't make it work in other way.

So, the class name is still hardcoded, but i kept the class-prefix parameter in case we would want to improve that feature eventually.

@cproensa

cproensa Apr 6, 2017

Contributor

I guess that enableDropzone( classPrefix, autoUpload ) was written with the idea to allow different class names to match the dropzone elements. For example, allowing more than one DZ, for different forms, in the same page.
However, i think that the library is internally dependant on elements with "dropzone" class. At least, i couldn't make it work in other way.

So, the class name is still hardcoded, but i kept the class-prefix parameter in case we would want to improve that feature eventually.

@cproensa

This comment has been minimized.

Show comment
Hide comment
@cproensa

cproensa Apr 6, 2017

Contributor

First time dealing with this library, so i need help reviewing and testing this.

Contributor

cproensa commented Apr 6, 2017

First time dealing with this library, so i need help reviewing and testing this.

@syncguru

Looks good overall. I think we need to get rid of auto-upload code though.

}
$( 'form .dropzone' ).each(function(){
var classPrefix = 'dropzone';
var autoUpload = $(this).hasClass('auto-dropzone');

This comment has been minimized.

@syncguru

syncguru Apr 6, 2017

Contributor

I thought we removed auto-upload, right? It used to be in issue details page but it is gone now.

@syncguru

syncguru Apr 6, 2017

Contributor

I thought we removed auto-upload, right? It used to be in issue details page but it is gone now.

This comment has been minimized.

@cproensa

cproensa Apr 6, 2017

Contributor

auto-upload is not used currently.
Supporting the feature as is, is simple enough, in case is needed, or reused by plugins?

edit: i mean, with this PR, it worksfine , by adding the "auto-dropzone" class. But it's not used within core pages

@cproensa

cproensa Apr 6, 2017

Contributor

auto-upload is not used currently.
Supporting the feature as is, is simple enough, in case is needed, or reused by plugins?

edit: i mean, with this PR, it worksfine , by adding the "auto-dropzone" class. But it's not used within core pages

@dregad

dregad approved these changes Apr 7, 2017

👍, didn't actually test.

@vboctor vboctor merged commit 4f38c83 into mantisbt:master Apr 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment