Skip to content

Commit

Permalink
Add animation to sweep progress bars with bootstrap class. Resolves d…
Browse files Browse the repository at this point in the history
  • Loading branch information
flarnie committed Mar 29, 2014
1 parent c54b65c commit 31330ff
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 1 deletion.
4 changes: 3 additions & 1 deletion app/helpers/sweeps_helper.rb
Expand Up @@ -36,7 +36,9 @@ def sweep_progress_bar(sweep)
auto_refresh_type: 'sweep',
auto_refresh_id: sweep.id,
}
content_tag(:div, class: 'progress',
klass = "progress"
klass += " progress-striped active" if sweep.count_pending > 0

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

This is a style nitpick, but we prefer single quotes unless you need double quotes for string interpolation or if your string has a single quote in it. However, if you take my suggestion below, this becomes irrelevant on this particular bit of code.

content_tag(:div, class: klass,

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

content_tag can accept an array for the class option. Although this isn't really that big of a deal, I think that would be preferable to the string concatenation method you have here, mostly because it feels more flexible.

classes = %w[progress]
classes += %w[progress-striped active] if sweep.count_pending > 0
content_tag(:div, class: classes,
title: sweep_status(sweep),
data: data_attrs) do
PROGRESS_BAR_STYLE_MAPPINGS.map do |state, bootstrap_class|
Expand Down
31 changes: 31 additions & 0 deletions spec/helpers/sweeps_helper_spec.rb
Expand Up @@ -50,4 +50,35 @@
it { should == '3 accepted, 2 rejected' }
end
end

describe '#sweep_progress_bar' do
let(:counts) do
{
pending: 10,
rejected: 0,
accepted: 0,
under_review: 0,
}

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

I don't think you gain much by putting all of these in a hash like this. Since you only ever modify the pending variable in your specs below, what do you think about using a let just for that variable and inlining the others in the sweep let?

let(:sweep) do
  build :sweep, count_pending:      pending,
                count_rejected:     0,
                count_accepted:     0,
                count_under_review: 0
context 'with pending snapshots' do
  let(:pending) { 10 }
  ...
end

context 'with all snapshots completed' do
  let(:pending) { 0 }
  ...
end
end
let(:sweep) do
build :sweep, count_rejected: counts[:rejected],
count_pending: counts[:pending],
count_accepted: counts[:accepted],
count_under_review: counts[:under_review]
end

let(:progress_bar) { sweep_progress_bar sweep }

context 'with pending snapshots' do
it "should have the progress-striped class" do

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

We avoid using the word "should" in our spec descriptions. Although I don't agree with everything on this site, Better Specs has this recommendation among others. Here's some discussion on this recommendation: betterspecs/betterspecs#15

expect(progress_bar[0..44]).to eq '<div class="progress progress-striped active"'

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

This string matching is pretty brittle. It would be better to use something like Nokogiri to parse the fragment and verify that it has the class in the correct place, or to use something like Capybara's have_css matcher.

Since this case is pretty straightforward, you could likely get away with using include?, but one of the aforementioned strategies would be preferable.

end
end
context 'with all snapshots completed' do

This comment has been minimized.

Copy link
@lencioni

lencioni Mar 30, 2014

We would normally leave a blank line between these two context blocks.

it "should only have the progress class" do
counts[:pending] = 0
expect(progress_bar[0..20]).to eq '<div class="progress"'
end
end
end
end

0 comments on commit 31330ff

Please sign in to comment.