Fixed addFile form when uploading with no files selected
Review Request #5803 — Created May 12, 2014 and discarded
Information | |
---|---|
salam0smy | |
Review Board | |
master | |
3338 | |
|
|
634ebba... | |
Reviewers | |
reviewboard, students | |
If a file was not selected and the user clicks Upload button, the form sends an empty submit request. The form then changes to a loading state, while its waiting for a response. Since the request is empty, the form never gets a response.
1 - If you click Upload button without selecting files, the modal box will gets destroyed: this can be fixed in gravy.modalBox.
2 - The Upload button is enabled when files are selected.
2a - The form is submitted normally when clicking the Upload button.
3 - The Upload button is disabled when files are not selected.
Description | From | Last Updated |
---|---|---|
Make sure that the first sentence here is capitalized and ends in a period. Please also wrap this to fit … |
|
|
Javascript should be indented 4 spaces. Your indentation here is inconsistent. |
|
|
All of this could be one statement: $('input[type="button"][value="Upload"]') .attr('disabled', $('#id_path').get(0).files.length > 0); Please also include quotes around the type/value in … |
|
|
It might be nice to just define the function inline here, instead of giving it a name. |
|
|
Add a blank line before this. |
|
|
Trailing whitespace. |
|
|
Add a space between () and { |
|
|
This needs to be indented to align with 'disabled' on the line above it (since it's another parameter to the … |
|
|
Trailing whitespace. |
|
|
.attr(...) should go on the next line. |
|
|
This is somewhat expensive. Can we fetch the input up-front and hold onto that, so we don't have to search … |
|
-
-
reviewboard/static/rb/js/common.js (Diff revision 1) I bet that reviewboard people are pretty picky about code styles (well, they are building a code review tool after all :) ), so most likely you should fix indentation. I haven't found reviewboard's javascript conventions, but this is probably a pretty similar guide: http://javascript.crockford.com/code.html
Change Summary:
disable or enable the upload button on files selected
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||
Commit: |
|
||||||||||||||||||
Diff: |
Revision 2 (+14 -1) |
-
-
reviewboard/static/rb/js/common.js (Diff revision 2) Make sure that the first sentence here is capitalized and ends in a period. Please also wrap this to fit in 80 columns.
-
reviewboard/static/rb/js/common.js (Diff revision 2) Javascript should be indented 4 spaces. Your indentation here is inconsistent.
-
reviewboard/static/rb/js/common.js (Diff revision 2) All of this could be one statement:
$('input[type="button"][value="Upload"]') .attr('disabled', $('#id_path').get(0).files.length > 0);
Please also include quotes around the type/value in the jquery selector.
-
reviewboard/static/rb/js/common.js (Diff revision 2) It might be nice to just define the function inline here, instead of giving it a name.
-
-
Please also fix your summary, description, and testing done to use full sentences with capitalization and punctuation.
Change Summary:
The code is rewritten to fit one statement.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+13 -1) |
-
-
-
-
reviewboard/static/rb/js/common.js (Diff revision 3) This needs to be indented to align with 'disabled' on the line above it (since it's another parameter to the
attr()
) -
Change Summary:
Fixed formatting issues
Summary: |
|
||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
||||||||||||||||||||||||
Testing Done: |
|
||||||||||||||||||||||||
Commit: |
|
||||||||||||||||||||||||
Diff: |
Revision 4 (+13 -1) |
Testing Done: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||
Diff: |
Revision 5 (+13) |
-
-
reviewboard/static/rb/js/common.js (Diff revision 5) This is somewhat expensive. Can we fetch the input up-front and hold onto that, so we don't have to search for it every time we get a change event from the text field?
Same with
id_path
.