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

Added AJAX behaviour to ATutor #1

Open
wants to merge 28 commits into
base: gsoc2013
from

Conversation

Projects
None yet
2 participants
@sdaityari
Owner

sdaityari commented Jun 20, 2013

File Uploads:
Added AJAX behaviour to add, edit and delete comments.

Forums:
Added AJAX behaviour to delete thread or reply.

Glossary:
Added AJAX behavior to add, edit and delete terms.

Issues Solved in this pull request:
5256: http://atutor.ca/atutor/mantis/view.php?id=5256
5276: http://atutor.ca/atutor/mantis/view.php?id=5276

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 10, 2013

you should reuse $_SERVER['HTTP_X_REQUESTED_WITH']. assign a variable to it and use in that IF statement

anvk commented on include/lib/vital_funcs.inc.php in b44d6b6 Jun 10, 2013

you should reuse $_SERVER['HTTP_X_REQUESTED_WITH']. assign a variable to it and use in that IF statement

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 10, 2013

Better put it out of the closure for readability purposes. This way a developer will see immediately what namespaces are used.

anvk commented on jscripts/ajax/FileStorageComments.js in b44d6b6 Jun 10, 2013

Better put it out of the closure for readability purposes. This way a developer will see immediately what namespaces are used.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 10, 2013

You should look for other ways of displaying an error message. Using alert() is highly inadvisable since it halts any JavaScript execution on the page as well as it is inaccessible.

anvk commented on jscripts/ajax/FileStorageComments.js in b44d6b6 Jun 10, 2013

You should look for other ways of displaying an error message. Using alert() is highly inadvisable since it halts any JavaScript execution on the page as well as it is inaccessible.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 10, 2013

You should follow the same syntax in the code by choosing to use only double quotes or single quotes. ATutor project uses double quotes all over the files JavaScript and PHP. I recommend to stick to this convention.

anvk commented on jscripts/ajax/FileStorageComments.js in b44d6b6 Jun 10, 2013

You should follow the same syntax in the code by choosing to use only double quotes or single quotes. ATutor project uses double quotes all over the files JavaScript and PHP. I recommend to stick to this convention.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 10, 2013

Maybe instead of defining ATutor.fileStorage.deleteComment you can do the modular code:

(function (fileStorage) {

// use fileStorage here

fileStorage.deleteComment = function (ot, ...
})(ATutor.fileStorage);

anvk commented on jscripts/ajax/FileStorageComments.js in b44d6b6 Jun 10, 2013

Maybe instead of defining ATutor.fileStorage.deleteComment you can do the modular code:

(function (fileStorage) {

// use fileStorage here

fileStorage.deleteComment = function (ot, ...
})(ATutor.fileStorage);

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 11, 2013

do not hardcode #ajax-response-dialog in your code.

anvk commented on jscripts/ajax/FileStorage.js in ba08935 Jun 11, 2013

do not hardcode #ajax-response-dialog in your code.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 11, 2013

Every time you call $("#ajax-response-dialog"), your code will trigger jQuery function which would find this element in a whole DOM. Get $("#ajax-response-dialog") only once and assign it to a variable and then use this variable to call html(). It is one of the major issue with current web-dev. Lots of developers call $('selector') constantly in their code which in the end triggers constant lookups in a DOM which is a very expensive task.

anvk commented on jscripts/ajax/FileStorage.js in ba08935 Jun 11, 2013

Every time you call $("#ajax-response-dialog"), your code will trigger jQuery function which would find this element in a whole DOM. Get $("#ajax-response-dialog") only once and assign it to a variable and then use this variable to call html(). It is one of the major issue with current web-dev. Lots of developers call $('selector') constantly in their code which in the end triggers constant lookups in a DOM which is a very expensive task.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 11, 2013

Ok here is a major issue with the code. Whenever I will see "this" in your code most likely I will be VERY grumpy.
Please Google online about common pitfalls with keyword "this". It is a huge pitfall in the world of webdev.
Here is a good link to look at for starters http://stackoverflow.com/questions/3127429/javascript-this-keyword
Your code here is the case #3. You will execute close() on the window object !!

anvk commented on jscripts/ajax/FileStorage.js in ba08935 Jun 11, 2013

Ok here is a major issue with the code. Whenever I will see "this" in your code most likely I will be VERY grumpy.
Please Google online about common pitfalls with keyword "this". It is a huge pitfall in the world of webdev.
Here is a good link to look at for starters http://stackoverflow.com/questions/3127429/javascript-this-keyword
Your code here is the case #3. You will execute close() on the window object !!

@sdaityari

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 13, 2013

Owner

Self note:

$("

", {
title: ajaxResponse,
id: "ajax-response-dialog"
}).appendTo(body);

Owner

sdaityari commented on jscripts/ajax/FileStorage.js in 1c4e21b Jun 13, 2013

Self note:

$("

", {
title: ajaxResponse,
id: "ajax-response-dialog"
}).appendTo(body);

@sdaityari

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 13, 2013

Owner

Wrong comment. I mean clicking "Cancel" under a textarea.

Owner

sdaityari commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 13, 2013

Wrong comment. I mean clicking "Cancel" under a textarea.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

I think if you want to ignore your request and take care of the issue in this branch then I suggest you to use queryDB here.

I think if you want to ignore your request and take care of the issue in this branch then I suggest you to use queryDB here.

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 14, 2013

Owner

I know.I had some problem with it. It's basically the same query and the result is not coming. Does something in that commented line look wrong to you as compared to line 61?

Owner

sdaityari replied Jun 14, 2013

I know.I had some problem with it. It's basically the same query and the result is not coming. Does something in that commented line look wrong to you as compared to line 61?

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

Well the first thing which comes into my mind - run the first query and log it somewhere. Then run the second query(queryDB) and log it somewhere as well. Compare. I can give you 100% that you just mistyped something somewhere.

Same comment as above - Stick to one convention in your php code. It should be either double quote or single quote. (and for ATutor PHP I think it is single quote)

anvk replied Jun 14, 2013

Well the first thing which comes into my mind - run the first query and log it somewhere. Then run the second query(queryDB) and log it somewhere as well. Compare. I can give you 100% that you just mistyped something somewhere.

Same comment as above - Stick to one convention in your php code. It should be either double quote or single quote. (and for ATutor PHP I think it is single quote)

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

First of all you need to comment on your arguments: ot, oid, file_id, id. Then try to avoid any underscores and use only CamelCase. So that submit_yes will become submitYes, file_id will become fileId

anvk commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 14, 2013

First of all you need to comment on your arguments: ot, oid, file_id, id. Then try to avoid any underscores and use only CamelCase. So that submit_yes will become submitYes, file_id will become fileId

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 14, 2013

Owner

I don't know where I got the convention, but I use CamelCase for variables or functions and underscores for keys (or variables that I send via AJAX to PHP).

If you think it is wrong, I can change it though.

Owner

sdaityari replied Jun 14, 2013

I don't know where I got the convention, but I use CamelCase for variables or functions and underscores for keys (or variables that I send via AJAX to PHP).

If you think it is wrong, I can change it though.

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

Stick with the one convention always. Even if you have some system which uses a mix convention like you have right now it can be against you one day. Not only in coding perspective but also human factor perspective. Developers who stick to strict rules usually have more attention to details and care more about the code... just giving you a good advice.

anvk replied Jun 14, 2013

Stick with the one convention always. Even if you have some system which uses a mix convention like you have right now it can be against you one day. Not only in coding perspective but also human factor perspective. Developers who stick to strict rules usually have more attention to details and care more about the code... just giving you a good advice.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

I think you need to do something tricky but beautiful here at the same time.
You should pass only 1 argument into this function and call it "options". This options object will consist of the following parameters: ot, oid, fileId, id, deleteTitle. You will need to leave a comment about options argument so that it would be easy to understand for developers.
Then you should do variable guards:
options = options || {};
Also you will define a defaults object inside of this function of type:
var defaults = {
deleteTitle: "Delete Comment"
// maybe define some defaults for ot, oid, fileId, id here.
};

options = $.extend({}, defaults, options, {
deleteUrl: "mods/_standard/file_storage/ajax/delete_comment.php"
});

This is a great way of defining some complicated functions which require lots of parameters. Another advantage is defaulting since your code becomes very flexible this way. Let's say if tomorrow I want to reuse the dialog somewhere else but change the title - > I can simply do it by passing it into the function. If not then it will be defaulted.
Unfortunately there is only 1 good default variable to see that -> deleteTitle. But in a more complex situations you could see a clear advantage.

anvk commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 14, 2013

I think you need to do something tricky but beautiful here at the same time.
You should pass only 1 argument into this function and call it "options". This options object will consist of the following parameters: ot, oid, fileId, id, deleteTitle. You will need to leave a comment about options argument so that it would be easy to understand for developers.
Then you should do variable guards:
options = options || {};
Also you will define a defaults object inside of this function of type:
var defaults = {
deleteTitle: "Delete Comment"
// maybe define some defaults for ot, oid, fileId, id here.
};

options = $.extend({}, defaults, options, {
deleteUrl: "mods/_standard/file_storage/ajax/delete_comment.php"
});

This is a great way of defining some complicated functions which require lots of parameters. Another advantage is defaulting since your code becomes very flexible this way. Let's say if tomorrow I want to reuse the dialog somewhere else but change the title - > I can simply do it by passing it into the function. If not then it will be defaulted.
Unfortunately there is only 1 good default variable to see that -> deleteTitle. But in a more complex situations you could see a clear advantage.

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 14, 2013

Owner

Yes, about this. I think I read about it somewhere (after I wrote this code) and was planning to change it soon (about the parameters).

Owner

sdaityari replied Jun 14, 2013

Yes, about this. I think I read about it somewhere (after I wrote this code) and was planning to change it soon (about the parameters).

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

in JSON files you have to follow the notation "key": value.
In JS you can drop double quotes to keep your code smaller and simpler.

anvk commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 14, 2013

in JSON files you have to follow the notation "key": value.
In JS you can drop double quotes to keep your code smaller and simpler.

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 14, 2013

Owner

Sure.

Owner

sdaityari replied Jun 14, 2013

Sure.

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

I would suggest the same thing as ^

// comment on how to use options
fileStorage.editCommentSubmit = function (options) {
options = options || {};
...
var parameters = $.extend({}, options, {
comment: $("textarea-" + options.id).val(),
editSubmit: true
});

...
};

anvk commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 14, 2013

I would suggest the same thing as ^

// comment on how to use options
fileStorage.editCommentSubmit = function (options) {
options = options || {};
...
var parameters = $.extend({}, options, {
comment: $("textarea-" + options.id).val(),
editSubmit: true
});

...
};

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 14, 2013

What's up with single quotes in a code ?

Let's decide on single quotes or double quotes in our JavaScript code and stick to it.
To be honest I'm good to go with any of those variations. If you want to use single quotes it is fine but you need to be CONSISTENT in the code.

anvk commented on jscripts/ajax/FileStorage.js in 54bc9e9 Jun 14, 2013

What's up with single quotes in a code ?

Let's decide on single quotes or double quotes in our JavaScript code and stick to it.
To be honest I'm good to go with any of those variations. If you want to use single quotes it is fine but you need to be CONSISTENT in the code.

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 14, 2013

Owner

My bad. I will changed them

Owner

sdaityari replied Jun 14, 2013

My bad. I will changed them

@anvk

This comment has been minimized.

Show comment
Hide comment
@anvk

anvk Jun 15, 2013

I know it is not a final version of your code. But do not be sloppy with empty lines. If it would be a pull request I would ban in no time because of those commit changes.

anvk commented on jscripts/ajax/FileStorage.js in 338aa04 Jun 15, 2013

I know it is not a final version of your code. But do not be sloppy with empty lines. If it would be a pull request I would ban in no time because of those commit changes.

This comment has been minimized.

Show comment
Hide comment
@sdaityari

sdaityari Jun 15, 2013

Owner

Oh, I had configured text editor (vim) to remove trailing whitespaces on exit. I think there was some issue with that. I will check on it.

Owner

sdaityari replied Jun 15, 2013

Oh, I had configured text editor (vim) to remove trailing whitespaces on exit. I think there was some issue with that. I will check on it.

Show outdated Hide outdated include/lib/vital_funcs.inc.php Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
};
//Setting title and message for dialog box according to thread/reply
if (options.ppid === "0") {

This comment has been minimized.

@anvk

anvk Jun 22, 2013

any reason ppid is a string?

@anvk

anvk Jun 22, 2013

any reason ppid is a string?

This comment has been minimized.

@sdaityari

sdaityari Jun 22, 2013

Owner

Since I use selectors like $("#something" + id), I pass the variables as strings the JS functions. PHP takes care of it in its end, I believe.

@sdaityari

sdaityari Jun 22, 2013

Owner

Since I use selectors like $("#something" + id), I pass the variables as strings the JS functions. PHP takes care of it in its end, I believe.

Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated include/lib/vital_funcs.inc.php Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/FileStorage.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Show outdated Hide outdated jscripts/ajax/Glossary.js Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment