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

Replacing legacy functions with UJS #795

Merged
merged 29 commits into from Jul 30, 2012

Conversation

Projects
None yet
6 participants
@jeffling
Member

jeffling commented Jul 25, 2012

In this branch, we replaced all deprecated javascript functions like link_to_remote, button_to_remote and remote_function with non-deprecated functions. Remote_function is now replaced using Ajax.Request. Link_to_remote and button_to_remote are now using link_to and button_to, respectively.

Related to #475 and #603

daneshd and others added some commits Mar 25, 2012

Merge branch 'issue475-remove-prototype-helper' of https://github.com…
…/daneshd/Markus into ujs

Conflicts:
	app/views/submission_rules/grace_period/_grader_tab.html.erb
added AUTH_TOKEN javascript constant since we aren't using remote_fun…
…ction to generate it anymore. ujsed up graders/_boot.js.erb
@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Jul 25, 2012

Member

Hi,

Very good work :)

All tests pass for me. Good to be merged for me.

Anyone ?

Member

benjaminvialle commented Jul 25, 2012

Hi,

Very good work :)

All tests pass for me. Good to be merged for me.

Anyone ?

@NelleV

View changes

Show outdated Hide outdated app/controllers/assignments_controller.rb
@@ -34,7 +34,7 @@ class AssignmentsController < ApplicationController
# Action called via Rails' remote_function from the test_result_window partial
# Prepares test result and updates content in window.
def render_test_result
@assignment = Assignment.find(params[:aid])
@assignment = Assignment.find(params[:aid])

This comment has been minimized.

@NelleV

NelleV Jul 25, 2012

Member

Please avoid trying to align '='. It's not maintainable long term. The usual convention is one space on each side of a mathematical sign.

@NelleV

NelleV Jul 25, 2012

Member

Please avoid trying to align '='. It's not maintainable long term. The usual convention is one space on each side of a mathematical sign.

This comment has been minimized.

@jeffling

jeffling Jul 25, 2012

Member

Understood :)

@jeffling

jeffling Jul 25, 2012

Member

Understood :)

@NelleV

View changes

Show outdated Hide outdated app/controllers/assignments_controller.rb
@@ -51,8 +51,8 @@ def render_test_result
def student_interface
@assignment = Assignment.find(params[:id])
@student = current_user
@grouping = @student.accepted_grouping_for(@assignment.id)
@student = current_user

This comment has been minimized.

@NelleV

NelleV Jul 25, 2012

Member

Same as above !

@NelleV

NelleV Jul 25, 2012

Member

Same as above !

@NelleV

View changes

Show outdated Hide outdated app/views/grade_entry_forms/_grades_table_filters.html.erb
:complete => "ap_thinking_stop();") -%>
:onchange =>
"new Ajax.Request(
'" + url_for({:action => :g_table_paginate, :id => grade_entry_form.id, :filter => filter, :sort_by => sort_by}) +"',

This comment has been minimized.

@NelleV

NelleV Jul 25, 2012

Member

This line is very long ( > 80char). Could you split it into several line with the correct indentation ?

@NelleV

NelleV Jul 25, 2012

Member

This line is very long ( > 80char). Could you split it into several line with the correct indentation ?

@NelleV

View changes

Show outdated Hide outdated app/views/grade_entry_forms/_grades_table_filters.html.erb
new Ajax.Request('" + url_for({:action => :g_table_paginate, :id => grade_entry_form.id,
:filter => filter, :sort_by => sort_by}) +
"',
{asynchronous:true, evalScripts:true, onComplete:function(request){ap_thinking_stop();}, parameters:'per_page=' + this.options[this.selectedIndex].value + '&update_alpha_pagination_options=' + true})"

This comment has been minimized.

@NelleV

NelleV Jul 25, 2012

Member

Same here.

@NelleV

NelleV Jul 25, 2012

Member

Same here.

@jerboaa

View changes

Show outdated Hide outdated app/views/layouts/plain.html.erb
@@ -3,7 +3,8 @@
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
<%= csrf_meta_tag %>
<%= csrf_meta_tag %>
<%= javascript_tag "const AUTH_TOKEN = #{form_authenticity_token.inspect};" if protect_against_forgery? %>

This comment has been minimized.

@jerboaa

jerboaa Jul 25, 2012

Member

This line seems to repeat many times. I'm not super fond of this code duplication. Could we add this somewhere more prominently? Perhaps in a layout definition?

@jerboaa

jerboaa Jul 25, 2012

Member

This line seems to repeat many times. I'm not super fond of this code duplication. Could we add this somewhere more prominently? Perhaps in a layout definition?

This comment has been minimized.

@jeffling

jeffling Jul 25, 2012

Member

I agree, is there a header layout file that's featured in all pages? I couldn't figure which file did that, so I just added the meta_tag and related to all files that contained . It would be annoying to maintain in the future.

@jeffling

jeffling Jul 25, 2012

Member

I agree, is there a header layout file that's featured in all pages? I couldn't figure which file did that, so I just added the meta_tag and related to all files that contained . It would be annoying to maintain in the future.

This comment has been minimized.

@jerboaa

jerboaa Jul 26, 2012

Member

Perhaps put in into the main layout. I forget what the name of it was. It's not "main" the other one :)

@jerboaa

jerboaa Jul 26, 2012

Member

Perhaps put in into the main layout. I forget what the name of it was. It's not "main" the other one :)

This comment has been minimized.

@jeffling

jeffling Jul 28, 2012

Member

Right now i'm making a new partial for things in tags that are called often. Any objections/comments?

@jeffling

jeffling Jul 28, 2012

Member

Right now i'm making a new partial for things in tags that are called often. Any objections/comments?

<%= remote_function :url => populate_repo_browser_assignment_submissions_path(
new Ajax.Request('<%= populate_repo_browser_assignment_submissions_path(

This comment has been minimized.

@jerboaa

jerboaa Jul 25, 2012

Member

We seem to have a few inconstistencies with using xxxx_path helpers and url_for() calls. The former is preferred. Do you mind changing it for the ones you are changing anyway? Thanks!

@jerboaa

jerboaa Jul 25, 2012

Member

We seem to have a few inconstistencies with using xxxx_path helpers and url_for() calls. The former is preferred. Do you mind changing it for the ones you are changing anyway? Thanks!

This comment has been minimized.

@jerboaa

jerboaa Jul 25, 2012

Member

Her'e's a bit of explanation of this: http://blog.markusproject.org/?p=3827

@jerboaa

jerboaa Jul 25, 2012

Member

Her'e's a bit of explanation of this: http://blog.markusproject.org/?p=3827

This comment has been minimized.

@jeffling

jeffling Jul 28, 2012

Member

hey, mike and I were talking about the routes and we think that we should get this fixed merged now, and changed all the url_fors in another branch. What do you think?

@jeffling

jeffling Jul 28, 2012

Member

hey, mike and I were talking about the routes and we think that we should get this fixed merged now, and changed all the url_fors in another branch. What do you think?

This comment has been minimized.

@daneshd

daneshd Jul 28, 2012

Contributor

Ah just seeing this now, feel free to ignore my comments below.

I think having the url_for's and the *_path functions pulled into a separate ticket is fine. I just hope they aren't forgotten about

@daneshd

daneshd Jul 28, 2012

Contributor

Ah just seeing this now, feel free to ignore my comments below.

I think having the url_for's and the *_path functions pulled into a separate ticket is fine. I just hope they aren't forgotten about

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Jul 25, 2012

Member

Nice work!

Member

jerboaa commented Jul 25, 2012

Nice work!

@benjaminvialle

View changes

Show outdated Hide outdated app/views/groups/index.html.erb
@@ -385,9 +378,9 @@ $("global_action_form").observe('ajax:success', function(evt, status, data, xhr)
<%= I18n.t("groups.groupe_name") %>
<input type="text" maxlength="30" id="new_group_name"></input>
</fieldset>
<p class="p_modal">
<p class="p_modal">

This comment has been minimized.

@benjaminvialle

benjaminvialle Jul 28, 2012

Member

Nitpick: Is it possible to remove all blank spaces at end of lines ?

In order to keep the code as clean as possible.

Thx :)

@benjaminvialle

benjaminvialle Jul 28, 2012

Member

Nitpick: Is it possible to remove all blank spaces at end of lines ?

In order to keep the code as clean as possible.

Thx :)

@daneshd

This comment has been minimized.

Show comment
Hide comment
@daneshd

daneshd Jul 28, 2012

Contributor

The rest of this looks good to me too.

Contributor

daneshd commented Jul 28, 2012

The rest of this looks good to me too.

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Jul 29, 2012

Member

Feel free to merge. Ship it!

Member

jerboaa commented Jul 29, 2012

Feel free to merge. Ship it!

benjaminvialle added a commit that referenced this pull request Jul 30, 2012

Merge pull request #795 from ummu/ujs
Replacing legacy functions with UJS

@benjaminvialle benjaminvialle merged commit 1f087c7 into MarkUsProject:master Jul 30, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment