From e59aad6e83cbdafcaf100bb86f6fb925f2fb779e Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Fri, 19 Jun 2015 02:01:53 -0400
Subject: [PATCH] Refactor LineHighlighter
---
.../javascripts/line_highlighter.js.coffee | 70 ++++++++-----------
.../line_highlighter_spec.js.coffee | 63 ++++++-----------
2 files changed, 51 insertions(+), 82 deletions(-)
diff --git a/app/assets/javascripts/line_highlighter.js.coffee b/app/assets/javascripts/line_highlighter.js.coffee
index a60a04783ac..d7de5516421 100644
--- a/app/assets/javascripts/line_highlighter.js.coffee
+++ b/app/assets/javascripts/line_highlighter.js.coffee
@@ -26,9 +26,13 @@
# </pre>
# </div>
# </div>
+#
class @LineHighlighter
+ # CSS class applied to highlighted lines
+ highlightClass: 'hll'
+
# Internal copy of location.hash so we're not dependent on `location` in tests
- @_hash = ''
+ _hash: ''
# Initialize a LineHighlighter object
#
@@ -41,7 +45,7 @@ class @LineHighlighter
unless hash == ''
range = @hashToRange(hash)
- unless isNaN(range[0])
+ if range[0]
@highlightRange(range)
# Scroll to the first highlighted line on initial load
@@ -69,7 +73,7 @@ class @LineHighlighter
lineNumber = $(event.target).data('line-number')
current = @hashToRange(@_hash)
- if isNaN(current[0]) or !event.shiftKey
+ unless current[0] and event.shiftKey
# If there's no current selection, or there is but Shift wasn't held,
# treat this like a single-line selection.
@setHash(lineNumber)
@@ -85,7 +89,7 @@ class @LineHighlighter
# Unhighlight previously highlighted lines
clearHighlight: ->
- $('.hll').removeClass('hll')
+ $(".#{@highlightClass}").removeClass(@highlightClass)
# Convert a URL hash String into line numbers
#
@@ -93,60 +97,44 @@ class @LineHighlighter
#
# Examples:
#
- # hashToRange('#L5') # => [5, NaN]
+ # hashToRange('#L5') # => [5, null]
# hashToRange('#L5-15') # => [5, 15]
- # hashToRange('#foo') # => [NaN, NaN]
+ # hashToRange('#foo') # => [null, null]
#
# Returns an Array
hashToRange: (hash) ->
- first = parseInt(hash.replace(/^#L(\d+)/, '$1'))
- last = parseInt(hash.replace(/^#L\d+-(\d+)/, '$1'))
+ matches = hash.match(/^#?L(\d+)(?:-(\d+))?$/)
+
+ if matches and matches.length
+ first = parseInt(matches[1])
+ last = matches[2] and parseInt(matches[2]) or null
- [first, last]
+ [first, last]
+ else
+ [null, null]
# Highlight a single line
#
- # lineNumber - Number to highlight. Must be parsable as an Integer.
- #
- # Returns undefined if lineNumber is not parsable as an Integer.
- highlightLine: (lineNumber) ->
- return if isNaN(parseInt(lineNumber))
-
- $("#LC#{lineNumber}").addClass('hll')
+ # lineNumber - Line number to highlight
+ highlightLine: (lineNumber) =>
+ $("#LC#{lineNumber}").addClass(@highlightClass)
# Highlight all lines within a range
#
- # range - An Array of starting and ending line numbers.
- #
- # Examples:
- #
- # # Highlight lines 5 through 15
- # highlightRange([5, 15])
- #
- # # The first value is required, and must be a number
- # highlightRange(['foo', 15]) # Invalid, returns undefined
- # highlightRange([NaN, NaN]) # Invalid, returns undefined
- #
- # # The second value is optional; if omitted, only highlights the first line
- # highlightRange([5, NaN]) # Valid
- #
- # Returns undefined if the first line is NaN.
+ # range - Array containing the starting and ending line numbers
highlightRange: (range) ->
- return if isNaN(range[0])
-
- if isNaN(range[1])
- @highlightLine(range[0])
- else
+ if range[1]
for lineNumber in [range[0]..range[1]]
@highlightLine(lineNumber)
+ else
+ @highlightLine(range[0])
+ # Set the URL hash string
setHash: (firstLineNumber, lastLineNumber) =>
- return if isNaN(parseInt(firstLineNumber))
-
- if isNaN(parseInt(lastLineNumber))
- hash = "#L#{firstLineNumber}"
- else
+ if lastLineNumber
hash = "#L#{firstLineNumber}-#{lastLineNumber}"
+ else
+ hash = "#L#{firstLineNumber}"
@_hash = hash
@__setLocationHash__(hash)
diff --git a/spec/javascripts/line_highlighter_spec.js.coffee b/spec/javascripts/line_highlighter_spec.js.coffee
index d9a1ff2d5bb..14fa487ff7f 100644
--- a/spec/javascripts/line_highlighter_spec.js.coffee
+++ b/spec/javascripts/line_highlighter_spec.js.coffee
@@ -13,6 +13,7 @@ describe 'LineHighlighter', ->
beforeEach ->
fixture.load('line_highlighter.html')
@class = new LineHighlighter()
+ @css = @class.highlightClass
@spies = {
__setLocationHash__: spyOn(@class, '__setLocationHash__').and.callFake ->
}
@@ -20,12 +21,12 @@ describe 'LineHighlighter', ->
describe 'behavior', ->
it 'highlights one line given in the URL hash', ->
new LineHighlighter('#L13')
- expect($('#LC13')).toHaveClass('hll')
+ expect($('#LC13')).toHaveClass(@css)
it 'highlights a range of lines given in the URL hash', ->
new LineHighlighter('#L5-25')
- expect($('.hll').length).toBe(21)
- expect($("#LC#{line}")).toHaveClass('hll') for line in [5..25]
+ expect($(".#{@css}").length).toBe(21)
+ expect($("#LC#{line}")).toHaveClass(@css) for line in [5..25]
it 'scrolls to the first highlighted line on initial load', ->
spy = spyOn($, 'scrollTo')
@@ -50,14 +51,14 @@ describe 'LineHighlighter', ->
describe 'without shiftKey', ->
it 'highlights one line when clicked', ->
clickLine(13)
- expect($('#LC13')).toHaveClass('hll')
+ expect($('#LC13')).toHaveClass(@css)
it 'unhighlights previously highlighted lines', ->
clickLine(13)
clickLine(20)
- expect($('#LC13')).not.toHaveClass('hll')
- expect($('#LC20')).toHaveClass('hll')
+ expect($('#LC13')).not.toHaveClass(@css)
+ expect($('#LC20')).toHaveClass(@css)
it 'sets the hash', ->
spy = spyOn(@class, 'setHash').and.callThrough()
@@ -75,8 +76,8 @@ describe 'LineHighlighter', ->
describe 'without existing highlight', ->
it 'highlights the clicked line', ->
clickLine(13, shiftKey: true)
- expect($('#LC13')).toHaveClass('hll')
- expect($('.hll').length).toBe(1)
+ expect($('#LC13')).toHaveClass(@css)
+ expect($(".#{@css}").length).toBe(1)
it 'sets the hash', ->
spy = spyOn(@class, 'setHash')
@@ -87,14 +88,14 @@ describe 'LineHighlighter', ->
it 'uses existing line as last line when target is lesser', ->
clickLine(20)
clickLine(15, shiftKey: true)
- expect($('.hll').length).toBe(6)
- expect($("#LC#{line}")).toHaveClass('hll') for line in [15..20]
+ expect($(".#{@css}").length).toBe(6)
+ expect($("#LC#{line}")).toHaveClass(@css) for line in [15..20]
it 'uses existing line as first line when target is greater', ->
clickLine(5)
clickLine(10, shiftKey: true)
- expect($('.hll').length).toBe(6)
- expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+ expect($(".#{@css}").length).toBe(6)
+ expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
describe 'with existing multi-line highlight', ->
beforeEach ->
@@ -103,26 +104,26 @@ describe 'LineHighlighter', ->
it 'uses target as first line when it is less than existing first line', ->
clickLine(5, shiftKey: true)
- expect($('.hll').length).toBe(6)
- expect($("#LC#{line}")).toHaveClass('hll') for line in [5..10]
+ expect($(".#{@css}").length).toBe(6)
+ expect($("#LC#{line}")).toHaveClass(@css) for line in [5..10]
it 'uses target as last line when it is greater than existing first line', ->
clickLine(15, shiftKey: true)
- expect($('.hll').length).toBe(6)
- expect($("#LC#{line}")).toHaveClass('hll') for line in [10..15]
+ expect($(".#{@css}").length).toBe(6)
+ expect($("#LC#{line}")).toHaveClass(@css) for line in [10..15]
describe '#hashToRange', ->
beforeEach ->
@subject = @class.hashToRange
it 'extracts a single line number from the hash', ->
- expect(@subject('#L5')).toEqual([5, NaN])
+ expect(@subject('#L5')).toEqual([5, null])
it 'extracts a range of line numbers from the hash', ->
expect(@subject('#L5-15')).toEqual([5, 15])
- it 'returns [NaN, NaN] when the hash is not a line number', ->
- expect(@subject('#foo')).toEqual([NaN, NaN])
+ it 'returns [null, null] when the hash is not a line number', ->
+ expect(@subject('#foo')).toEqual([null, null])
describe '#highlightLine', ->
beforeEach ->
@@ -130,36 +131,16 @@ describe 'LineHighlighter', ->
it 'highlights the specified line', ->
@subject(13)
- expect($('#LC13')).toHaveClass('hll')
+ expect($('#LC13')).toHaveClass(@css)
it 'accepts a String-based number', ->
@subject('13')
- expect($('#LC13')).toHaveClass('hll')
-
- it 'returns undefined when given NaN', ->
- expect(@subject(NaN)).toBe(undefined)
- expect(@subject('foo')).toBe(undefined)
-
- describe '#highlightRange', ->
- beforeEach ->
- @subject = @class.highlightRange
-
- it 'returns undefined when first line is NaN', ->
- expect(@subject([NaN, 15])).toBe(undefined)
- expect(@subject(['foo', 15])).toBe(undefined)
-
- it 'returns undefined when given an invalid first line', ->
- expect(@subject(['foo', 15])).toBe(undefined)
- expect(@subject([NaN, NaN])).toBe(undefined)
- expect(@subject('foo')).toBe(undefined)
+ expect($('#LC13')).toHaveClass(@css)
describe '#setHash', ->
beforeEach ->
@subject = @class.setHash
- it 'returns undefined when given an invalid first line', ->
- expect(@subject('foo', 15)).toBe(undefined)
-
it 'sets the location hash for a single line', ->
@subject(5)
expect(@spies.__setLocationHash__).toHaveBeenCalledWith('#L5')
--
GitLab