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

Refactor code to eventually remove jquery dependency - part 1 #538

Open
sashadev-sky opened this issue Jan 27, 2020 · 4 comments · May be fixed by #540
Open

Refactor code to eventually remove jquery dependency - part 1 #538

sashadev-sky opened this issue Jan 27, 2020 · 4 comments · May be fixed by #540

Comments

@sashadev-sky
Copy link
Collaborator

@sashadev-sky sashadev-sky commented Jan 27, 2020

First Time?

This is a first-timers-only issue. This means we've worked to make it more legible to folks who either haven't contributed to our codebase before, or even folks who haven't contributed to open source before.

If that's you, we're interested in helping you take the first step and can answer questions and help you out as you do. Note that we're especially interested in contributions from people from groups underrepresented in free and open source software!

We know that the process of creating a pull request is the biggest barrier for new contributors. This issue is for you 💝

If you have contributed before, consider leaving this one for someone new, and looking through our general help wanted issues. Thanks!

🤔 What you will need to know.

Nothing. This issue is meant to welcome you to Open Source :) We are happy to walk you through the process.

Problem

What happens:

JQuery is the 2nd biggest resource we are pulling in (first is Leaflet.js), slowing down page loading time and we barely even use it.

Solution

Let's start refactoring any jQuery find in the code base to vanilla JS. In this issue, we will tackle the below code. Please refactor the ajax implementation to use XHR instead.

startExport: function() {
return new Promise(function(resolve) {
var opts = this._exportOpts;
opts.resolve = resolve; // allow resolving promise in user-defined functions, to stop spinner on completion
var statusUrl;
var self = this;
this.updateInterval = null;
// this may be overridden to update the UI to show export progress or completion
function _defaultUpdater(data) {
data = JSON.parse(data);
// optimization: fetch status directly from google storage:
if (data.status_url) {
if (statusUrl !== data.status_url && data.status_url.match('.json')) {
// if (data.status_url && data.status_url.substr(0,1) === "/") {
// opts.statusUrl = opts.statusUrl + data.status_url;
// } else {
statusUrl = data.status_url;
// }
}
if (data.status === 'complete') {
clearInterval(self.updateInterval);
if (!self.customCollection) {
self._exportOpts.collection = undefined;
}
resolve();
if (data.jpg !== null) {
alert('Export succeeded. ' + opts.exportUrl + data.jpg);
}
}
// TODO: update to clearInterval when status == "failed" if we update that in this file:
// https://github.com/publiclab/mapknitter-exporter/blob/main/lib/mapknitterExporter.rb
console.log(data);
}
}
// receives the URL of status.json, and starts running the updater to repeatedly fetch from status.json;
// this may be overridden to integrate with any UI
function _defaultHandleStatusRes(data) {
statusUrl = opts.statusUrl + data;
// repeatedly fetch the status.json
self.updateInterval = setInterval(function intervalUpdater() {
$.ajax(statusUrl + '?' + Date.now(), {// bust cache with timestamp
type: 'GET',
crossDomain: true,
}).done(function(data) {
opts.updater(data);
});
}, opts.frequency);
}
// initiate the export
function _defaultFetchStatusUrl(opts) {
$.ajax({
url: opts.exportStartUrl,
crossDomain: true,
type: 'POST',
data: {
collection: JSON.stringify(opts.collection),
scale: opts.scale,
upload: true,
},
success: function(data) { opts.handleStatusRes(data); }, // this handles the initial response
});
}
// If the user has passed collection property
if (opts.collection) {
self.customCollection = true;
} else {
self.customCollection = false;
opts.collection = this._group.generateExportJson().images;
}
opts.frequency = opts.frequency || 3000;
opts.scale = opts.scale || 100; // switch it to _getAvgCmPerPixel !
opts.updater = opts.updater || _defaultUpdater;
opts.handleStatusRes = opts.handleStatusRes || _defaultHandleStatusRes;
opts.fetchStatusUrl = opts.fetchStatusUrl || _defaultFetchStatusUrl;
opts.fetchStatusUrl(opts);
}.bind(this));
},

Step by Step

  • Claim this issue with a comment here, below, and ask any clarifying questions you need
  • Fork the repository and set it up locally following the main repo README instructions https://github.com/publiclab/Leaflet.DistortableImage
  • Create a new feature branch with a unique name descriptive to the issue
  • Try to fix the issue following the steps above, but even before you're done, you can:
    commit your changes to your branch and start a pull request (see contributing to Public Lab software) but mark it as "in progress" if you have questions or if you haven't finished
  • Reference this issue in your pull request body
  • Once you submit your pull request, if there's an additional checklist provided for getting it merged, get those boxes checked off. Either way, mention me @sashadev-sky for a review.

💬 Get help

If you need any help - here are some options:

@sifihog

This comment has been minimized.

Copy link

@sifihog sifihog commented Jan 27, 2020

Hello,
I'd like to give this a try, if possible. I've not worked on any open source projects before.

@Odisseuss

This comment has been minimized.

Copy link

@Odisseuss Odisseuss commented Jan 28, 2020

Hello @sashadev-sky, has anyone taken up this issue yet?

@carlo-ev

This comment has been minimized.

Copy link

@carlo-ev carlo-ev commented Jan 28, 2020

Hey I want to give this issue a go, would be my first open source contribution. Hope its open still didn't see it on pull requests.

@sabina-rohman

This comment has been minimized.

Copy link

@sabina-rohman sabina-rohman commented Feb 13, 2020

Hi, I am new to open source contributions. Please, assign it to me if it's not assigned to anybody. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.