-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Migrate CoffeeScript to JavaScript #3653
Conversation
} | ||
}, | ||
showFeasibilityFieldsOnChange: function() { | ||
$("#valuation_budget_investment_edit_form input[type=radio][name='budget_investment[feasibility]']").change(function() { |
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.
Line 27 exceeds the maximum line length of 110 max-len
}, | ||
showFeasibilityFields: function() { | ||
var feasibility; | ||
feasibility = $("#valuation_budget_investment_edit_form input[type=radio][name='budget_investment[feasibility]']:checked").val(); |
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.
Line 19 exceeds the maximum line length of 110 max-len
element: this, | ||
viewerExtensions: [App.LegislationAnnotatable.viewerExtension], | ||
editorExtensions: [App.LegislationAnnotatable.editorExtension] | ||
}).include(App.LegislationAnnotatable.scrollToAnchor).include(App.LegislationAnnotatable.addWeightClasses).include(annotator.storage.http, { |
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.
Line 218 exceeds the maximum line length of 110 max-len
var current_user_id; | ||
$(document).off("renderLegislationAnnotation").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); | ||
$(document).off("click", "[data-annotation-id]").on("click", "[data-annotation-id]", App.LegislationAnnotatable.onClick); | ||
$(document).off("click", "[data-cancel-annotation]").on("click", "[data-cancel-annotation]", function(e) { |
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.
Line 195 exceeds the maximum line length of 110 max-len
initialize: function() { | ||
var current_user_id; | ||
$(document).off("renderLegislationAnnotation").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); | ||
$(document).off("click", "[data-annotation-id]").on("click", "[data-annotation-id]", App.LegislationAnnotatable.onClick); |
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.
Line 194 exceeds the maximum line length of 110 max-len
}, | ||
initialize: function() { | ||
var current_user_id; | ||
$(document).off("renderLegislationAnnotation").on("renderLegislationAnnotation", App.LegislationAnnotatable.renderAnnotationComments); |
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.
Line 193 exceeds the maximum line length of 110 max-len
$.event.trigger({ | ||
type: "renderLegislationAnnotation", | ||
annotation_id: ann_id, | ||
annotation_url: el.closest(".legislation-annotatable").data("legislation-annotatable-base-url"), |
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.
Line 152 exceeds the maximum line length of 110 max-len
5ae1e47
to
1fcb1db
Compare
ba8311e
to
793b80e
Compare
}); | ||
} else { | ||
$(e.target).find("label").addClass("error"); | ||
$("<small class='error'>" + data.responseJSON[0] + "</small>").insertAfter($(e.target).find("textarea")); |
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.
Line 123 exceeds the maximum line length of 110 max-len
dataType: "script" | ||
}).done((function() { | ||
$("#new_legislation_annotation #legislation_annotation_quote").val(this.annotation.quote); | ||
$("#new_legislation_annotation #legislation_annotation_ranges").val(JSON.stringify(this.annotation.ranges)); |
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.
Line 104 exceeds the maximum line length of 110 max-len
event.preventDefault(); | ||
event.stopPropagation(); | ||
if (App.LegislationAnnotatable.isMobile()) { | ||
annotation_url = $(event.target).closest(".legislation-annotatable").data("legislation-annotatable-base-url"); |
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.
Line 55 exceeds the maximum line length of 110 max-len
App.Imageable.clearPreview(data); | ||
$("#new_image_link").removeClass("hide"); | ||
$(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); |
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.
Line 151 exceeds the maximum line length of 110 max-len
}, | ||
setPreview: function(data) { | ||
var image_preview; | ||
image_preview = "<div class='small-12 column text-center image-preview'><figure><img src='" + data.result.attachment_url + "' class='cached-image'></figure></div>"; |
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.
Line 126 exceeds the maximum line length of 110 max-len
"Save": "Guardar", | ||
"Edit": "Editar", | ||
"Delete": "Borrar", | ||
"Unregistered": "<p>Necesitas <a href='/users/sign_in'>iniciar sesión</a> o <a href='/users/sign_up'>registrarte</a> para continuar.</p>" |
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.
Line 14 exceeds the maximum line length of 110 max-len
App.Documentable.clearProgressBar(data); | ||
App.Documentable.unlockUploads(); | ||
$(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); |
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.
Line 147 exceeds the maximum line length of 110 max-len
App.Documentable.clearInputErrors(data); | ||
$(data.addAttachmentLabel).hide(); | ||
$(data.wrapper).find(".attachment-actions").removeClass("small-12").addClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").removeClass("small-3").addClass("small-12"); |
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.
Line 60 exceeds the maximum line length of 110 max-len
}, | ||
add_reply: function(parent_id, response_html) { | ||
if ($("#" + parent_id + " .comment-children").length === 0) { | ||
$("#" + parent_id).append("<li><ul id='" + parent_id + "_children' class='no-bullet comment-children'></ul></li>"); |
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.
Line 11 exceeds the maximum line length of 110 max-len
1fcb1db
to
c0e1738
Compare
793b80e
to
e88e449
Compare
5f49387
to
e240e98
Compare
e88e449
to
245e7d9
Compare
} | ||
}, | ||
showFeasibilityFieldsOnChange: function() { | ||
$("#valuation_budget_investment_edit_form input[type=radio][name='budget_investment[feasibility]']").change(function() { |
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.
Line 26 exceeds the maximum line length of 110 max-len
}, | ||
showFeasibilityFields: function() { | ||
var feasibility; | ||
feasibility = $("#valuation_budget_investment_edit_form input[type=radio][name='budget_investment[feasibility]']:checked").val(); |
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.
Line 18 exceeds the maximum line length of 110 max-len
showUsernameMessage = function(response) { | ||
var klass; | ||
klass = response.available ? "no-error" : "error"; | ||
usernameInput.after($("<small class=\"" + klass + "\" style=\"margin-top: -16px;\">" + response.message + "</small>")); |
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.
Line 13 exceeds the maximum line length of 110 max-len
}); | ||
}, | ||
initializeMap: function(element) { | ||
var addMarkerInvestments, clearFormfields, createMarker, editable, getPopupContent, latitudeInputSelector, longitudeInputSelector, map, mapAttribution, mapCenterLatLng, mapCenterLatitude, mapCenterLongitude, mapTilesProvider, marker, markerIcon, markerLatitude, markerLongitude, moveOrPlaceMarker, openMarkerPopup, removeMarker, removeMarkerSelector, updateFormfields, zoom, zoomInputSelector; |
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.
Line 15 exceeds the maximum line length of 110 max-len
dataType: "script" | ||
}).done((function() { | ||
$("#new_legislation_annotation #legislation_annotation_quote").val(this.annotation.quote); | ||
$("#new_legislation_annotation #legislation_annotation_ranges").val(JSON.stringify(this.annotation.ranges)); |
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.
Line 103 exceeds the maximum line length of 110 max-len
event.preventDefault(); | ||
event.stopPropagation(); | ||
if (App.LegislationAnnotatable.isMobile()) { | ||
annotation_url = $(event.target).closest(".legislation-annotatable").data("legislation-annotatable-base-url"); |
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.
Line 54 exceeds the maximum line length of 110 max-len
App.Imageable.clearPreview(data); | ||
$("#new_image_link").removeClass("hide"); | ||
$(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); |
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.
Line 150 exceeds the maximum line length of 110 max-len
}, | ||
setPreview: function(data) { | ||
var image_preview; | ||
image_preview = "<div class='small-12 column text-center image-preview'><figure><img src='" + data.result.attachment_url + "' class='cached-image'></figure></div>"; |
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.
Line 125 exceeds the maximum line length of 110 max-len
App.Imageable.clearInputErrors(data); | ||
$(data.addAttachmentLabel).hide(); | ||
$(data.wrapper).find(".attachment-actions").removeClass("small-12").addClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").removeClass("small-3").addClass("small-12"); |
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.
Line 60 exceeds the maximum line length of 110 max-len
e240e98
to
febe71d
Compare
245e7d9
to
72c3706
Compare
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.
Some files could not be reviewed due to errors:
.eslintrc:
.eslintrc: Configuration for rule "eqeqeq" is invalid: Value {"":"ignore"} should NOT have additional properties. Value ["always",{"":"ignore"}] should NOT have more than 1 items. Value ["always",{"":"ignore"}] should match some schema in anyOf.Error: .eslintrc:
at validateRuleOptions (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:119:19)
at Object.keys.forEach.id (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:162:9)
at Array.forEach ()
at validateRules (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:161:30)
at Object.validate (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:239:5)
at loadFromDisk (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:516:19)
at Object.load (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:559:20)
at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:227:44)
at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:179:43)
at Config.getConfigVector (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:286:21)
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.
Some files could not be reviewed due to errors:
.eslintrc:
.eslintrc: Configuration for rule "eqeqeq" is invalid: Value {"":"ignore"} should NOT have additional properties. Value ["always",{"":"ignore"}] should NOT have more than 1 items. Value ["always",{"":"ignore"}] should match some schema in anyOf.Error: .eslintrc:
at validateRuleOptions (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:119:19)
at Object.keys.forEach.id (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:162:9)
at Array.forEach ()
at validateRules (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:161:30)
at Object.validate (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:239:5)
at loadFromDisk (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:516:19)
at Object.load (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:559:20)
at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:227:44)
at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:179:43)
at Config.getConfigVector (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:286:21)
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.
Some files could not be reviewed due to errors:
.eslintrc:
.eslintrc: Configuration for rule "eqeqeq" is invalid: Value {"":"ignore"} should NOT have additional properties. Value ["always",{"":"ignore"}] should NOT have more than 1 items. Value ["always",{"":"ignore"}] should match some schema in anyOf.Error: .eslintrc:
at validateRuleOptions (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:119:19)
at Object.keys.forEach.id (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:162:9)
at Array.forEach ()
at validateRules (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:161:30)
at Object.validate (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:239:5)
at loadFromDisk (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:516:19)
at Object.load (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:559:20)
at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:227:44)
at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:179:43)
at Config.getConfigVector (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:286:21)
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.
Some files could not be reviewed due to errors:
.eslintrc:
.eslintrc: Configuration for rule "eqeqeq" is invalid: Value {"":"ignore"} should NOT have additional properties. Value ["always",{"":"ignore"}] should NOT have more than 1 items. Value ["always",{"":"ignore"}] should match some schema in anyOf.Error: .eslintrc:
at validateRuleOptions (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:119:19)
at Object.keys.forEach.id (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:162:9)
at Array.forEach ()
at validateRules (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:161:30)
at Object.validate (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-validator.js:239:5)
at loadFromDisk (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:516:19)
at Object.load (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config/config-file.js:559:20)
at Config.getLocalConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:227:44)
at Config.getConfigHierarchy (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:179:43)
at Config.getConfigVector (/home/linters/app/versions/eslint-4.18.2/node_modules/eslint/lib/config.js:286:21)
72c3706
to
ba409c1
Compare
App.Documentable.clearProgressBar(data); | ||
App.Documentable.unlockUploads(); | ||
$(data.wrapper).find(".attachment-actions").addClass("small-12").removeClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").addClass("small-3").removeClass("small-12"); |
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.
Line 146 exceeds the maximum line length of 110 max-len
App.Documentable.clearInputErrors(data); | ||
$(data.addAttachmentLabel).hide(); | ||
$(data.wrapper).find(".attachment-actions").removeClass("small-12").addClass("small-6 float-right"); | ||
$(data.wrapper).find(".attachment-actions .action-remove").removeClass("small-3").addClass("small-12"); |
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.
Line 59 exceeds the maximum line length of 110 max-len
}, | ||
add_reply: function(parent_id, response_html) { | ||
if ($("#" + parent_id + " .comment-children").length === 0) { | ||
$("#" + parent_id).append("<li><ul id='" + parent_id + "_children' class='no-bullet comment-children'></ul></li>"); |
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.
Line 10 exceeds the maximum line length of 110 max-len
febe71d
to
efe1160
Compare
ba409c1
to
eef7b84
Compare
efe1160
to
5392646
Compare
f2f8e7e
to
692afcb
Compare
5392646
to
798e90b
Compare
692afcb
to
461eceb
Compare
798e90b
to
de7f0d5
Compare
c3174a8
to
f8ad278
Compare
124911c
to
412fc10
Compare
412fc10
to
ab27bfd
Compare
f8ad278
to
eb277f0
Compare
eb277f0
to
d8be18d
Compare
Compiled using `coffee -c` with CoffeeScript 1.12.6.
These statements were automatically added by CoffeeScript. I'm only removing the obvious cases; there might be more cases where the `return` statement isn't necessary.
For now we're only adding rules related to spacing and double quotes, following the same rules we use in Ruby, which are the same rules CoffeeScript followed when compiling these files. We're also using the recommended ESLint rules, which will warn us about many JavaScript common pitfalls, the `strict` rule which enforces using strict mode, and the `no-console` rule, which will prevent us from shipping code meant for debugging. Although it's arguably more common to use the JSON format to define these rules, I've chosen YAML because it's the format we use in all our linters.
We originally wrote `undefined` in CoffeeScript. CoffeeScript compiled it to `void 0` when generating the JavaScript files. However, the reason to do so was the `undefined` variable was mutable before ECMAScript 5. Using `undefined` is more intuitive, and nowadays there's no reason to use `void 0`.
While I personally prefer the "never" option for this rule, we haven't discussed which guideline to follow, so for now I'm applying the rule CoffeeScript used when generating these files.
I'm using the same criteria we use in Ruby: 110 characters, and the severity set as a warning, since it's a rule we break sometimes.
Using `==` and `!=` sometimes causes unexpected behaviour in JavaScript, and it's easy for Ruby developers to use them incorrectly.
The rule is not the most important one, but the name is awesome!
Using the same variable name makes the code more difficult to read. We're also enabling the `no-param-reassing` rule since we accidentally reassigned params in commit 824dd26, and this rule will prevent us from doing so in the future.
It's very easy for Ruby developers to omit the `return` statement in an array callback, such as the ones in `map` or `filter`. Doing so will make those funcions have the same effect as `forEach`, which is usually not the intended behaviour.
We were using the dot notation in most places, but not everywhere.
Using `Array(0, 1, 2)` will *not* create an Array with the elements [0, 1, 2]. This is a mistake I've seen in the past, and the JavaScript community recommends adding this rule.
ab27bfd
to
8231096
Compare
Migrate CoffeeScript to JavaScript
References
Depends on pull requests #3652, #3654 and #3651.
Background
CoffeeScript was a great language which helped avoiding some JavaScript pitfalls. Thanks to it, ECMAScript 6 was born.
However, nowaday there are way more developers who can write or understand JavaScript code than developers who can write or understand CoffeeScript code, particularly since ECMAScript 6 was released back in 2015.
Objectives
Notes
For now we aren't configuring babel nor any other transpiler, and aren't adding webpack nor any other JavaScript dependencies, so we're using ECMAScript 5 syntax and functionality to keep the same browser compatibility we had before these changes