Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix js animation not disable if load jquery in body #2440

Conversation

kuei0221
Copy link
Contributor

Animation disabler now can only turn off the animation come from jQuery if jQuery is loaded in head, because insert_disable place the template at the end of head.

def insert_disable(html)
html.sub(%r{(</head>)}, "#{disable_markup}\\1")
end

If jQuery is loaded with the body, jQuery will be undefined in head so the function won't work.

DISABLE_MARKUP_TEMPLATE = <<~HTML
<script defer="defer">
//<![CDATA[
(typeof jQuery !== 'undefined') && (jQuery.fx.off = true);

By placing it to the end of the body, developer do not have to worry about where they are loading jQuery and give a better experience 😄

Copy link
Member

@twalpole twalpole left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the original intent was for defer to handle this use case, but that isn't happening because defer is only for external scripts (ignored for inline). Please remove the defer attribute along when making fixes for this PR

@@ -405,7 +405,9 @@
@animation_session.visit('with_animation')
scroll_y = @animation_session.evaluate_script(<<~JS)
(function(){
window.scrollTo(0,500);
$('html, body').animate({
Copy link
Member

@twalpole twalpole Jan 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@why are you modifying this test? If you want to test different behavior add a new test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆗

@@ -77,6 +63,20 @@
<a href='#' class='animation' onclick='this.classList.add("away")' oncontextmenu='this.classList.add("pseudo")'>
animate me away
</a>
<script src="/jquery.js" type="text/javascript" charset="utf-8"></script>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a new view is needed for new tests please create it rather than modifying the existing one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🆗

@kuei0221 kuei0221 force-pushed the fix/js_animation_not_disable_if_load_jquery_in_body branch from 58129e0 to f95e581 Compare January 23, 2021 09:05
@twalpole
Copy link
Member

LGTM - thanks

@twalpole twalpole merged commit 958a179 into teamcapybara:master Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants