Clean up CoffeeScript code #3651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
houndci-bot
reviewed
Jul 1, 2019
| @@ -82,8 +81,7 @@ App.Map = | |||
| e.target.bindPopup(getPopupContent(data)).openPopup() | |||
|
|
|||
| getPopupContent = (data) -> | |||
| content = "<a href='/budgets/#{data["budget_id"]}/investments/#{data["investment_id"]}'>#{data["investment_title"]}</a>" | |||
| return content | |||
| "<a href='/budgets/#{data["budget_id"]}/investments/#{data["investment_id"]}'>#{data["investment_title"]}</a>" | |||
There was a problem hiding this comment.
Line exceeds maximum allowed length. Length is 116, max is 100.
| $("#new_legislation_annotation #legislation_annotation_quote").val(@annotation.quote) | ||
| $("#new_legislation_annotation #legislation_annotation_ranges").val(JSON.stringify(@annotation.ranges)) | ||
| $("#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.
Line exceeds maximum allowed length. Length is 115, max is 100.
javierm
force-pushed
the
simplify_coffeescript
branch
from
e1f47d8
to
4280a4b
Compare
houndci-bot
reviewed
Jul 1, 2019
| process_background = "[name='legislation_process[background_color]']" | ||
| process_font = ", [name='legislation_process[font_color]']" | ||
| processes = process_background + process_font | ||
| processes = "[name='legislation_process[background_color]'], [name='legislation_process[font_color]']" |
There was a problem hiding this comment.
Line exceeds maximum allowed length. Length is 106, max is 100.
javierm
force-pushed
the
simplify_coffeescript
branch
2 times, most recently
from
a49685e
to
d713068
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
from
d713068
to
c295528
Compare
javierm
force-pushed
the
remove_unused_javascript
branch
2 times, most recently
from
05e5789
to
5167622
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
from
c295528
to
4e4101b
Compare
javierm
force-pushed
the
remove_unused_javascript
branch
from
5167622
to
6dd720b
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
from
4e4101b
to
5ae1e47
Compare
javierm
force-pushed
the
remove_unused_javascript
branch
from
6dd720b
to
bd96ea9
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
from
5ae1e47
to
1fcb1db
Compare
javierm
changed the base branch from
remove_unused_javascript
to
fix_javascript_typos
javierm
force-pushed
the
fix_javascript_typos
branch
from
401189b
to
906d532
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
4 times, most recently
from
e240e98
to
febe71d
Compare
javierm
force-pushed
the
fix_javascript_typos
branch
from
906d532
to
3634266
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
2 times, most recently
from
efe1160
to
5392646
Compare
javierm
force-pushed
the
fix_javascript_typos
branch
from
3634266
to
49185f0
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
2 times, most recently
from
798e90b
to
de7f0d5
Compare
houndci-bot
reviewed
Jul 7, 2019
|
|
||
| initialize: -> | ||
| if App.WatchFormChanges.forms().length == 0 || App.WatchFormChanges.msg() == undefined | ||
| return | ||
|
|
||
| $(document).off("page:before-change").on("page:before-change", -> App.WatchFormChanges.checkChanges()) | ||
| $(document).off("page:before-change").on("page:before-change", App.WatchFormChanges.checkChanges) |
There was a problem hiding this comment.
Line exceeds maximum allowed length. Length is 101, max is 100.
javierm
force-pushed
the
fix_javascript_typos
branch
2 times, most recently
from
7b83d79
to
eab35dc
Compare
javierm
force-pushed
the
simplify_coffeescript
branch
from
f8ad278
to
eb277f0
Compare
There were functions which were just simple wrappers to common jQuery functions.
These methods were added in 2009 and are supported by 98.5% of the browsers, including Internet Explorer 9, 10 and 11. We were already using them in some places.
We're planning to replace CoffeeScript with JavaScript, and CoffeeScript compiles `for .. in` to complex JavaScript code.
The code won't do anything if there are no elements, so there's no need to check that condition.
The `append` method can handle arrays, so there's no need to loop through each element. There's more to improve on this method, like the `asc` variable being global to a table instead of depending on the current column, but for now I'm only refactoring and maintaining the current behaviour.
Including the table body is more intuitive than excluding the first row and the table foot.
Using the same name means from that point the name doesn't refer to the original parameter, which might be confusing when reading the method.
Iterating through the jQuery elements with `each` is more similar to other iterators we use. Furthermore, we avoid declaring the `index` variable we don't use.
Combining both the inline and "traditional" `if` styles made the code difficult to read, particularly when compiled to JavaScript.
In foundation extras we were assigning functions to local variables, but in the rest of the CoffeeScript files we use them as object properties.
We're using `this` most of the time, and it's what's used in JavaScript.
Strict mode is supported by 98% of the browsers, including Internet Explorer 10, and it helps developers avoid common JavaScript pitfalls.
The assignment operator can easily be mistaken with the equality operator when used in a condition, making the code more difficult to read.
In JavaScript we cannot easily repeat something N times, and
CoffeeScript's range loop compiles into complex JavaScript.
We could also use `Array.from({ length: N })`, but that's not supported
by old browsers like Internet Explorer 11.
Local variables are one of the things CoffeeScript doesn't compile to modern JavaScript automatically: it uses `var` instead of `const` or `let`. Besides, using `$this = $(this)` is usually done to reference the current object in another function where the current object is a different one. Here we were using it with no clear purpose.
We accidentally added some single quotes recently.
javierm
force-pushed
the
simplify_coffeescript
branch
from
eb277f0
to
d8be18d
Compare
smarques
pushed a commit
to venetochevogliamo/consul
that referenced
this pull request
Apr 29, 2020
…ript Clean up CoffeeScript code
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
References
Background
We're planning to convert all our CoffeeScript code to JavaScript. As a first step, this pull request cleans some of the CoffeeScript code up, and rewrites the parts which would generate complex JavaScript code.
Objectives