First pass at adding follow checkboxes to the top users page (issue 117) #129

Merged
merged 1 commit into from Sep 29, 2011

2 participants

@darrenaustin

Here is my initial stab at adding "following" checkboxes to the Top Users pages. Here are my issues and questions:

  • Currently it only works with javascript turned on. Not sure what to do with javascript turned off. Should we add a "Submit" button to the bottom of the "Following" column if we are not running with JS? Or perhaps we just don't show the checkboxes with JS off.

  • If the user is not logged in, I still show the "Following" column, but it will be empty. Should we remove it altogether in this case? If so, that might be tricky given the table setup.

  • I haven't AJAXified the "Follow" button on the profile page yet. I'll do that if everyone is comfortable with my approach.

  • I am not happy with the big let statement at the top of generate-user-list. Is there a cleaner way to get the current user-id its following list?

This is my first exposure to a clojure web app, so I would appreciate any feedback.

Thanks,
--Darren

@amalloy amalloy commented on the diff Sep 29, 2011
src/foreclojure/users.clj
- [:th "Username"]
- [:th "Problems Solved"]]]
- (map-indexed (fn [rownum {:keys [rank user contributor solved]}]
- [:tr (row-class rownum)
- [:td (rank-class rank) rank]
- [:td
- (when contributor [:span.contributor "* "])
- [:a.user-profile-link {:href (str "/user/" user)} user]]
- [:td.centered (count solved)]])
- user-set)]))
+ (let [[user-id following]
+ (if (session/session-get :user)
+ (with-user [{:keys [_id following]}]
+ [_id following])
+ [nil nil])]
+ (list
@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

This looks fine. Not an especially huge let by any means.

@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

If you convert following to a set here, you will have fast lookup up above in following-checkbox, rather than O(n) iteration through their followeing list.

@darrenaustin
darrenaustin added a line comment Sep 29, 2011

Nice catch on the set. I will make that change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amalloy amalloy commented on the diff Sep 29, 2011
src/foreclojure/users.clj
(response/redirect (str "/user/" username)))
+(defn rest-follow-user [username follow?]
+ (follow-user username follow?)
+ (json-str {"following" follow?
+ "next-action" (follow-url username (not follow?))
+ "next-label" (if follow? "Unfollow" "Follow")}))
+
@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

This stuff isn't used yet, right? But you're planning to when you AJAX it up? Just confirming my reading of the code.

@darrenaustin
darrenaustin added a line comment Sep 29, 2011

For the new checkboxes, they are already AJAX'd up and using this stuff. The "Follow" buttons on the profile pages haven't been AJAX'd yet. But that's mostly just a matter of add some code the foreclojure.js to hook the buttons up to an AJAX call.

@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

Ah, I glazed over the JS stuff at the beginning. Sneaky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amalloy amalloy commented on the diff Sep 29, 2011
resources/public/script/foreclojure.js
@@ -22,6 +22,22 @@ $(document).ready(function() {
$(this).parents("form").attr("action", "/problem/edit").submit();
});
+ $("form input.following").live("click", function(e) {
+ e.preventDefault();
+ var $checkbox = $(this)
+ var $form = $checkbox.parents("form")
+ $.ajax({type: "POST",
@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

Is it standard to use names like $checkbox for jquery stuff? I've never seen that before.

@darrenaustin
darrenaustin added a line comment Sep 29, 2011

Not sure if it is standard, but I think it is common for variables that hold jQuery DOM objects to be prefixed with a $. Not critical though and if we aren't using that convention elsewhere I will change it here.

@amalloy
4clojure member
amalloy added a line comment Sep 29, 2011

We're not, but I can definitely see how such a convention would be useful; I guess otherwise you can forget when you have real DOM objects and when you don't? @gigasquid, care to offer an opinion?

@gigasquid
4clojure member
gigasquid added a line comment Sep 30, 2011

I can see how the convention can be useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@amalloy amalloy merged commit e452d42 into 4clojure:develop Sep 29, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment