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

toolbox: avoid $.get when url is undefined #1325

Merged
merged 2 commits into from
May 31, 2018

Conversation

thelostone-mc
Copy link
Member

@thelostone-mc thelostone-mc commented May 31, 2018

Description
Issue

x1

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
  • js

@thelostone-mc thelostone-mc self-assigned this May 31, 2018
@ghost ghost added the in progress label May 31, 2018
@thelostone-mc
Copy link
Member Author

thelostone-mc commented May 31, 2018

Fixes the failed $.get() calls but still do we need to invoke it via setInterval ?

  $('.cards .img img').each(function(index, element) {
    document.preloads.push($(element).data('hover'));

  });

@owocki Can't we just directly do $.get here instead ?
Haven't changed this cause I wanna check why this was written this way (so that I don't break anything unknowingly )

@codecov
Copy link

codecov bot commented May 31, 2018

Codecov Report

Merging #1325 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1325   +/-   ##
=======================================
  Coverage   30.83%   30.83%           
=======================================
  Files         124      124           
  Lines        8738     8738           
  Branches     1146     1146           
=======================================
  Hits         2694     2694           
  Misses       5933     5933           
  Partials      111      111

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b1a42c...6eed03a. Read the comment docs.

@thelostone-mc thelostone-mc added bug This is something that isn't working as intended. needs review and removed in progress labels May 31, 2018
@mbeacom
Copy link
Contributor

mbeacom commented May 31, 2018

@kziemianek completed the original bounty for this. Any thoughts?

mbeacom
mbeacom previously approved these changes May 31, 2018
@SaptakS
Copy link
Contributor

SaptakS commented May 31, 2018

@thelostone-mc @mbeacom I am removing the $.get code for the time. Hope you both are okay with it?

Seems the $.get for document preloads
was actually not doing anything.
@mbeacom
Copy link
Contributor

mbeacom commented May 31, 2018

Sounds good!

Copy link
Contributor

@owocki owocki left a comment

Choose a reason for hiding this comment

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

SHIPPITTTT

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

:shipit:

@SaptakS SaptakS merged commit 9b3c26c into gitcoinco:master May 31, 2018
@ghost ghost removed the needs review label May 31, 2018
@mbeacom
Copy link
Contributor

mbeacom commented May 31, 2018

screenshot 2018-05-31 16 39 31

Beautiful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This is something that isn't working as intended.
Projects
None yet
Development

Successfully merging this pull request may close these issues.