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

#3 Find and hide spinner after the result gets displayed #18

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SwethaMuralidharan
Copy link

@SwethaMuralidharan SwethaMuralidharan commented Jan 20, 2018

#3 Find and hide spinner
Explanation:

  1. Found the form which contains the search bar.
  2. Found the class name "spinnable" in the form.
  3. Searched "spinnable" globally and found a "spinnable.js" file.
  4. There were 2 ajax calls. (ajax:before) & (ajax:after). Changed the "after" to "success" because thats the keyword for ajax on success.
  5. Also realised that the spinner.show() in index.js.erb is no longer required if these 2 ajax calls are correct. So deleted that too. Tested it. it worked.

@@ -1,2 +1,2 @@
$("#articleList").html('<%= raw escape_javascript(render(partial: "article_list", locals: { articles: @articles })) %>');
$("#spinner").show();
$("#spinner").hide();

Choose a reason for hiding this comment

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

This works, but there is a more elegant solution.

There is already something in the codebase that is supposed to run after the search ajax call completes. However, it is slightly broken. I would find where that code is and fix it, instead of adding new code in a separate file.

Choose a reason for hiding this comment

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

Actually, once you make this more elegant fix, you could get rid of this line in this file.

@@ -1,2 +1,2 @@
$("#articleList").html('<%= raw escape_javascript(render(partial: "article_list", locals: { articles: @articles })) %>');
$("#spinner").show();
$("#spinner").hide();

Choose a reason for hiding this comment

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

Actually, once you make this more elegant fix, you could get rid of this line in this file.

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

Successfully merging this pull request may close these issues.

3 participants