Add animation to sweep progress bars with bootstrap class. Resolves #77. #84
Conversation
|
Thanks for the pull request @paradasia! Overall the code looks pretty good and I'm glad that you included specs. I left some comments inline for your consideration. If you don't have the time or desire to polish things up, just let me know and I'll be happy to take over. One general comment is that it would be really great to have a better commit message. In fact, some of the details you included in your pull request could be added to the commit message. In my mind, a great commit message explains why the change is being made. Thoughts, motivations, attempts, drawbacks, edge cases, and other things like that are also good to include if you have any. Thanks for contributing! @trotzig and I are delighted. |
…iffux#77. Summary of changes: * Changes #sweep_progress_bar view helper by adding different classes to element depending on the number of pending sweeps. If pending sweeps are 0, (and the sweep is completed), then the class doesn't have animated stripes. * Adds 2 new specs to check that class is being appropriately applied depending on the number of pending sweeps. * All specs passing. Reason: * Striped animation makes progress bar more obvious and engaging. * Change from stripes to solid signals visually the sweep completion. Drawbacks: * New specs for this feature rely on existing HTML structure somewhat. * Specs for this feature may be more appropriate as feature specs; added to existing sweeps_helper_spec.rb since the current specs seem to focus on unit testing.
|
Thanks for reviewing the code and for your comments - I just didn't notice your inline comments, and am happy to make the changes you mentioned to the specs. I'm always happy to contribute and to improve my code. |
Add animation to sweep progress bars with bootstrap class. Resolves #77.
|
Thanks @paradasia, and welcome to the team of contributors! |
3 participants
Summary of changes: