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

Integration for: https://github.com/kartik-v/bootstrap-fileinput #383

Merged
merged 5 commits into from Jun 22, 2014

Conversation

Projects
None yet
4 participants
@subes
Copy link
Contributor

subes commented Jun 9, 2014

See #372

@buildhive

This comment has been minimized.

Copy link

buildhive commented Jun 9, 2014

Michael Haitz » wicket-bootstrap #932 SUCCESS
This pull request looks good
(what's this?)


import java.util.List;

import javax.annotation.concurrent.NotThreadSafe;

This comment has been minimized.

@martin-g

martin-g Jun 11, 2014

Collaborator

Which dependency brings this class ?
We need to use JDK 6 classes only.

This comment has been minimized.

@subes

subes Jun 11, 2014

Author Contributor

Ah right, just remove that annotation, its a pattern from java concurrency
in practice. You can find a jar via the name jcip annotations, but it seems
I just forgot to delete it.
Am 11.06.2014 11:37 schrieb "Martin Grigorov" notifications@github.com:

In
bootstrap-extensions/src/main/java/de/agilecoders/wicket/extensions/markup/html/bootstrap/fileinput/BootstrapFileUploadField.java:

@@ -0,0 +1,124 @@
+package de.agilecoders.wicket.extensions.markup.html.bootstrap.fileinput;
+
+import java.util.List;
+
+import javax.annotation.concurrent.NotThreadSafe;

Which dependency brings this class ?
We need to use JDK 6 classes only.


Reply to this email directly or view it on GitHub
https://github.com/l0rdn1kk0n/wicket-bootstrap/pull/383/files#r13640905.

This comment has been minimized.

@subes

subes Jun 11, 2014

Author Contributor

Can you reject this pull request? I will create a new one later.
Am 11.06.2014 11:48 schrieb "subes" gsubes@gmail.com:

Ah right, just remove that annotation, its a pattern from java concurrency
in practice. You can find a jar via the name jcip annotations, but it seems
I just forgot to delete it.
Am 11.06.2014 11:37 schrieb "Martin Grigorov" notifications@github.com:

In
bootstrap-extensions/src/main/java/de/agilecoders/wicket/extensions/markup/html/bootstrap/fileinput/BootstrapFileUploadField.java:

@@ -0,0 +1,124 @@
+package de.agilecoders.wicket.extensions.markup.html.bootstrap.fileinput;
+
+import java.util.List;
+
+import javax.annotation.concurrent.NotThreadSafe;

Which dependency brings this class ?
We need to use JDK 6 classes only.


Reply to this email directly or view it on GitHub
https://github.com/l0rdn1kk0n/wicket-bootstrap/pull/383/files#r13640905
.


public BootstrapFileUploadField(final String id, final IModel<List<FileUpload>> model) {
super(id, model);
ajaxBehavior = newAjaxFormSubmitBehavior(AJAX_EVENT_NAME);

This comment has been minimized.

@martin-g

martin-g Jun 11, 2014

Collaborator

Why ajaxBehavior is a member field ?
It seems it is not used out of the constructor.

And why this behavior has to be added to the FileUploadField at all ?
Usually the application decides whether it wants to submit the form with Ajax or not.
Please remove it from here.

This comment has been minimized.

@subes

subes Jun 11, 2014

Author Contributor

The behavior is needed to implement a workaround for the fileinput
javascript added upload button to work via ajax. You can only define a ajax
URL, but that one cannot handle multiupload. Thus I work around this by
catching the upload button click and invoking the ajax behaviors callback
via the self defined event for that particular fileuploadfield. If there is
no javascript enabled in the browser, the upload button will fallback to
using a normal post form submit that uploads the file. The ajaxBehavior is
needed as a field variable so that the callback url can be rendered
properly in the renderHead(...).
Also the newAjaxFormSubmitBehavior method is defined in order for a user to
be able to customize the ajax callback that is executed. e.g. to add ajax
attributes or override the submit method.

2014-06-11 11:48 GMT+02:00 Martin Grigorov notifications@github.com:

In
bootstrap-extensions/src/main/java/de/agilecoders/wicket/extensions/markup/html/bootstrap/fileinput/BootstrapFileUploadField.java:

+@NotThreadSafe
+public class BootstrapFileUploadField extends FileUploadField {
+

  • private static final String UPLOAD_BUTTON_CLASS = "fileinput-upload-button";
  • private static final String AJAX_EVENT_NAME = "fileinput-upload-button-clicked";
  • private final AjaxFormSubmitBehavior ajaxBehavior;
  • private boolean showCaption = false;
  • private boolean showPreview = true;
  • private boolean showRemove = false;
  • private boolean showUpload = true;
  • public BootstrapFileUploadField(final String id, final IModel<List> model) {
  •    super(id, model);
    
  •    ajaxBehavior = newAjaxFormSubmitBehavior(AJAX_EVENT_NAME);
    

Why ajaxBehavior is a member field ?
It seems it is not used out of the constructor.

And why this behavior has to be added to the FileUploadField at all ?
Usually the application decides whether it wants to submit the form with
Ajax or not.
Please remove it from here.


Reply to this email directly or view it on GitHub
https://github.com/l0rdn1kk0n/wicket-bootstrap/pull/383/files#r13641241.

This comment has been minimized.

@subes

subes Jun 11, 2014

Author Contributor

Well, indeed the ajaxBehavior field is not needed anymore, I had a previous
version in mind. Let me work over this to make it better.

2014-06-11 11:54 GMT+02:00 subes gsubes@gmail.com:

The behavior is needed to implement a workaround for the fileinput
javascript added upload button to work via ajax. You can only define a ajax
URL, but that one cannot handle multiupload. Thus I work around this by
catching the upload button click and invoking the ajax behaviors callback
via the self defined event for that particular fileuploadfield. If there is
no javascript enabled in the browser, the upload button will fallback to
using a normal post form submit that uploads the file. The ajaxBehavior is
needed as a field variable so that the callback url can be rendered
properly in the renderHead(...).
Also the newAjaxFormSubmitBehavior method is defined in order for a user
to be able to customize the ajax callback that is executed. e.g. to add
ajax attributes or override the submit method.

2014-06-11 11:48 GMT+02:00 Martin Grigorov notifications@github.com:

In

bootstrap-extensions/src/main/java/de/agilecoders/wicket/extensions/markup/html/bootstrap/fileinput/BootstrapFileUploadField.java:

+@NotThreadSafe
+public class BootstrapFileUploadField extends FileUploadField {
+

  • private static final String UPLOAD_BUTTON_CLASS = "fileinput-upload-button";
  • private static final String AJAX_EVENT_NAME = "fileinput-upload-button-clicked";
  • private final AjaxFormSubmitBehavior ajaxBehavior;
  • private boolean showCaption = false;
  • private boolean showPreview = true;
  • private boolean showRemove = false;
  • private boolean showUpload = true;
  • public BootstrapFileUploadField(final String id, final IModel<List> model) {
  •    super(id, model);
    
  •    ajaxBehavior = newAjaxFormSubmitBehavior(AJAX_EVENT_NAME);
    

Why ajaxBehavior is a member field ?
It seems it is not used out of the constructor.

And why this behavior has to be added to the FileUploadField at all ?
Usually the application decides whether it wants to submit the form with
Ajax or not.
Please remove it from here.


Reply to this email directly or view it on GitHub
https://github.com/l0rdn1kk0n/wicket-bootstrap/pull/383/files#r13641241
.

This comment has been minimized.

@martin-g

martin-g Jun 11, 2014

Collaborator

No need to close it.
Just commit to your branch and the Pull Request will update itself automatically.
Add a comment here when you are done and we will review it again.

Thanks again for your help!

@martin-g

This comment has been minimized.

Copy link
Collaborator

martin-g commented Jun 11, 2014

Thanks for this contribution!
See my comments from the review.
It would be nice to add a demo usage in the samples app so it is easy to see how to use for the users and easy for us to test whether it works when making a release.

@buildhive

This comment has been minimized.

Copy link

buildhive commented Jun 12, 2014

Michael Haitz » wicket-bootstrap #936 SUCCESS
This pull request looks good
(what's this?)

@subes

This comment has been minimized.

Copy link
Contributor Author

subes commented Jun 12, 2014

I have improved the file input and added a sample page (needed a clean page since there are conflicts when using jasny-input aswell on the same page). Hope it's now ok!

I also now made the ajax event specific to one file input only by making the event name unique.

@buildhive

This comment has been minimized.

Copy link

buildhive commented Jun 12, 2014

Michael Haitz » wicket-bootstrap #937 SUCCESS
This pull request looks good
(what's this?)

@buildhive

This comment has been minimized.

Copy link

buildhive commented Jun 12, 2014

Michael Haitz » wicket-bootstrap #938 SUCCESS
This pull request looks good
(what's this?)

@martin-g

This comment has been minimized.

Copy link
Collaborator

martin-g commented Jun 13, 2014

It seems you will need to create a new Pull Request after all.
There are many "changed" files which are not part of this task.
I guess the reason is the merge you did to update to master branch.

@subes

This comment has been minimized.

Copy link
Contributor Author

subes commented Jun 13, 2014

The updated files are just chmod changes because I developed the last part
on windows. It should be no problem to merge it.
Am 13.06.2014 09:26 schrieb "Martin Grigorov" notifications@github.com:

It seems you will need to create a new Pull Request after all.
There are many "changed" files which are not part of this task.
I guess the reason is the merge you did to update to master branch.


Reply to this email directly or view it on GitHub
#383 (comment)
.

martin-g added a commit that referenced this pull request Jun 14, 2014

martin-g added a commit that referenced this pull request Jun 14, 2014

Issue #383 - Use a Panel that wraps the file input with a Form so tha…
…t Ajax uploads do not process the whole outer form
@martin-g

This comment has been minimized.

Copy link
Collaborator

martin-g commented Jun 14, 2014

At https://github.com/l0rdn1kk0n/wicket-bootstrap/tree/subes-master you can see my improvements.

The main change is that I introduced a Panel that wraps the file input with a Form. This form is used as an inner form for the Ajax uploads (by using the Upload button). This way the upload doesn't process any other form components in the real/outer form.

@subes

This comment has been minimized.

Copy link
Contributor Author

subes commented Jun 14, 2014

+1

2014-06-14 22:06 GMT+02:00 Martin Grigorov notifications@github.com:

At https://github.com/l0rdn1kk0n/wicket-bootstrap/tree/subes-master you
can see my improvements.

The main change is that I introduced a Panel that wraps the file input
with a Form. This form is used as an inner form for the Ajax uploads (by
using the Upload button). This way the upload doesn't process any other
form components in the real/outer form.


Reply to this email directly or view it on GitHub
#383 (comment)
.

martin-g added a commit that referenced this pull request Jun 22, 2014

@martin-g martin-g merged commit b4a6a31 into l0rdn1kk0n:master Jun 22, 2014

@martin-g

This comment has been minimized.

Copy link
Collaborator

martin-g commented Jun 22, 2014

Please review the latest version of the new feature.
I've modified it to use wicket-jquery-selectors (JQuery + AbstractConfig) but I didn't added setters/getters for all possible settings. We can add them on request.

@subes

This comment has been minimized.

Copy link
Contributor Author

subes commented Jun 25, 2014

Sorry to reopen this, but can you make BootstrapFileInputField public? I would like to use that instead of BootstrapFileInput since I want my own markup to be used. Thanks!

EDIT: since this cannot be reopened, I created a new ticket for this: #393

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment