Remove dependency on JQuery for hiding the caret

[zofrex]

Summary

Remove dependency on jQuery in caret hiding code so it works on any website.

Motivation

Enabling Capybara::Screenshot.hide_caret = true on my Rails app gave the following error:

Error:
VisualsTest#test_0001_index:
Selenium::WebDriver::Error::JavascriptError: javascript error: $ is not defined
  (Session info: chrome=86.0.4240.75)
    test/system/visuals_test.rb:9:in `block in <class:VisualsTest>'

This is because there is an undeclared dependency on jQuery for this feature, and my website does not have jQuery installed.

Details

I've made an attempt to change this feature to not depend on jQuery. Please let me know what you think!

Of note is that this new behaviour is not 1:1 with the original. I believe the original visits every node in the DOM and applies a style to it, whereas this code declares the style once at the top level for all elements. It's possible that certain existing CSS styles could override this, but I went this route because it's simpler and I believe will be faster. If you'd prefer to preserve the existing behaviour I can take a shot at doing that without jQuery too, but I believe this approach should suffice in almost all cases (how often does anyone style the caret, let alone with !important?)

[donv]

Looks good. Can you fix the RuboCop offence to get the tests working?

It looks like the element is added for every screenshot. That would fill up the DOM, right? Adding it once should be enough.

[zofrex]

I'm not sure the best way to fix this Rubocop complaint ("Metrics/ModuleLength: Module has too many lines. [142/136]").

Is keeping this module under 136 lines strongly desired? I think it might need to be broken up if so, and as a newcomer it's not really obvious to me where that boundary would be drawn.

Can the limit be raised? If so, should I override it just for this module, or for all?

It looks like the element is added for every screenshot. That would fill up the DOM, right? Adding it once should be enough.

Good catch! You're right, it adds a new DOM element every time. I'll work on a fix for that.

[donv]

I'm sorry, I forgot to hint :)

Just bump the metrics limit in .rubocop-todo.yml for all modules.

[zofrex]

Tests pass & the DOM-filling issue is resolved. Are you happy with the code? Also, would you prefer I squash those two commits into one?

[pftg]

@zofrex is it possible to avoid rubocop changes and add tests to cover those changes?

[zofrex]

@pftg there isn't an obvious quick-win here for getting under that line limit. The JS could be squashed onto fewer lines, but I think that would be less readable. The line limit was really tight on this module already, so I think it would need to be broken up somehow to come under that limit. I don't really have enough context on this project to suggest where a suitable boundary might be for breaking it up, I'm afraid :)

As for testing – I agree it should be tested, but I don't know how. I don't see any prior art in this project to crib from for testing this kind of feature – in fact, you can delete every line of code in this file and all tests still pass! I think testing the stability features in general seems like a wider effort?

[pftg]

@zofrex thanks for the reply 👍

1. robocop: there is another pr which will remove this file so that change will be redundant.
2. test - ok, make sense, will think about how to test such stuff on the weekend

[pftg]

@donv could you check this PR

[donv]

Merged. Thanks!

[zofrex]

Thank you!