Display and navigate b/w organisations in the sidebar #12
Conversation
prabhanshuguptagit
changed the title
|
|
||
| (defn retrieve-by-user | ||
| [user-id] | ||
| {:success (vec (db/retrieve-by-user user-id))}) |
There was a problem hiding this comment.
Why the conversion to vector?
There was a problem hiding this comment.
This is to convert the db response from a clojure sequence to valid JSON for the HTTP response. I missed the fact that it's already being taken care of by the wrap-json-response middleware. Getting rid of this.
| @@ -49,6 +50,7 @@ | |||
| (:user @formdata) | |||
| (fn [res] | |||
| (swap! error assoc :message "") | |||
| (user-api/get-data!) | |||
There was a problem hiding this comment.
The on-click fn here is the same as that in login.cljs.
Maybe you can extract these out into one.
There was a problem hiding this comment.
Yes this bit needs some refactoring. I'm planning on doing that in a separate PR in #14 .
prabhanshuguptagit
force-pushed
the
sidebar-navigate-orgs
branch
from
753c3de to
e04f237
Compare
|
|
||
| (defn retrieve-by-user | ||
| [request] | ||
| (let [{user-id :id} (:identity request)] |
There was a problem hiding this comment.
| (let [{user-id :id} (:identity request)] | |
| (let [user-id (get-in request [:identity :id])] |
src/clj/villagebook/routes.clj
Outdated
| @@ -9,8 +9,9 @@ | |||
| [villagebook.config :as config])) | |||
|
|
|||
| (def apiroutes | |||
There was a problem hiding this comment.
| (def apiroutes | |
| (def api-routes |
| @@ -1,11 +1,18 @@ | |||
| (ns villagebookUI.api.organisation | |||
There was a problem hiding this comment.
Capitals in the ns name seems odd.
There was a problem hiding this comment.
Yes🤷♂ but decided to stick with it, changing would be too much effort.
src/cljs/villagebookUI/api/user.cljs
Outdated
| :error-handler error-handler-fn})) | ||
| {:handler handler-fn | ||
| :error-handler error-handler-fn | ||
| :finally finally-fn})) |
There was a problem hiding this comment.
I'd also just call them success-handler, error-handler, and finalizer. The -fn seems unnecessary, but ymmv.
| @@ -19,7 +20,10 @@ | |||
| [:form.inline-block {:on-submit (fn [e] | |||
There was a problem hiding this comment.
Extract this handler function out. Don't write more than a line of code inside the view, it isn't as tangible, and is not testable.
There was a problem hiding this comment.
(.preventDefault e) can stay perhaps, because it is specific to the view.
| @@ -46,6 +47,7 @@ | |||
| (:user @formdata) | |||
| (fn [res] | |||
| (swap! error assoc :message "") | |||
| (fetchers/fetch-user!) | |||
| @@ -27,7 +28,7 @@ | |||
| (let [formdata (r/atom {}) | |||
| error (r/atom {})] | |||
| (fn [] | |||
| (if @store/user | |||
| (if (user-store/read) | |||
There was a problem hiding this comment.
Structure this as a guard clause instead? Also, extract the else part into another function, login-form perhaps. This will improve the legibility of this function, and let you reuse the form elsewhere.
There was a problem hiding this comment.
I've refactored this here in a separate WIP PR.
| (doall | ||
| (for [org (org-store/get-all) | ||
| :let [id (:id org)]] | ||
| [:li.item {:key (:id org) |
There was a problem hiding this comment.
| [:li.item {:key (:id org) | |
| [:li.item {:key id |
| [:a.sidebar-link [utils/patch "#5fcc5f"] "Org 1"]] | ||
| [:li.item | ||
| [:a.sidebar-link [utils/patch "#ff8383"] "Org 2"]] | ||
| (doall |
There was a problem hiding this comment.
Are you sure the doall is necessary here?
There was a problem hiding this comment.
This is needed since for generates a lazy sequence. Unless the sequence is expanded in place, reagent can't know if an atom is being de-refed in the sequence and can't make re-renders based on it (org-store/get-selected in this case). Relevant thread
src/cljs/villagebookUI/fetchers.cljs
Outdated
| (defn fetch-user! [] | ||
| (user-api/get-data | ||
| (fn [res] | ||
| (user-store/add! res)) |
There was a problem hiding this comment.
Isn't this just user-store/add!? Why wrap another fn around it?
There was a problem hiding this comment.
user-store/add! only adds the user map to the state. fetch-user! makes a call to the user API and stores the result (a user map) in the state. Made this a separate function since this bit of code gets repeated at a number of places throughout the app.
No description provided.