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

Display and navigate b/w organisations in the sidebar #12

Merged
merged 18 commits into from
Oct 1, 2019
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 20 additions & 7 deletions resources/public/assets/css/style.css
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,12 @@ a:hover {
background: #2e3344;
transition: left 0.1s;
color: white;
overflow-y: scroll;
overflow: -moz-scrollbars-none;
}

.sidebar::-webkit-scrollbar {
width: 0 !important
}

.sidebar-logo-box {
Expand All @@ -160,15 +166,10 @@ a:hover {
margin-bottom: 4px;
}

.sidebar-section {
padding-left: 10px;
}

.sidebar-section-head {
color: white;
font-weight: bolder;
margin-bottom: 5px;
margin-top: 25px;
margin: 25px 0px 5px 8px;
}

.sidebar-section-list {
Expand All @@ -179,9 +180,19 @@ a:hover {
.sidebar-section-list > .item {
display: block;
line-height: 35px;
padding: 0 15px 0 15px;
padding: 0 15px 0 20px;
width: 100%;
text-decoration: none;
border: 2px solid transparent;
}

.sidebar-section-list .item:hover {
background: #363c4e;
}

.sidebar .item.active {
border-left: 2px solid aliceblue;
background: #404040;
}

.sidebar-section-list .item-label {
Expand All @@ -205,6 +216,7 @@ a:hover {
.sidebar-link {
color: #d5e2f1 !important;
cursor: pointer;
display: block;
}

.sidebar-link:hover {
Expand Down Expand Up @@ -256,6 +268,7 @@ a:hover {
margin-bottom: 0;
background-color: #fff;
box-shadow: 0px 2px rgba(0, 0, 0, 0.05), 0 1px 0 rgba(0, 0, 0, 0.05);
padding: 10px 10px 0 10px;
}

/* End Dashboard*/
5 changes: 5 additions & 0 deletions src/clj/villagebook/organisation/handlers.clj
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,8 @@
(if (s/valid? ::organisation-spec/id id)
(retrieve-from-model id)
(res/bad-request "Invalid request."))))

(defn retrieve-by-user
[request]
(let [{user-id :id} (:identity request)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(let [{user-id :id} (:identity request)]
(let [user-id (get-in request [:identity :id])]

(res/response (:success (models/retrieve-by-user user-id)))))
4 changes: 4 additions & 0 deletions src/clj/villagebook/organisation/models.clj
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,7 @@
(if org
{:success org}
{:error "Organisation not found"})))

(defn retrieve-by-user
[user-id]
{:success (db/retrieve-by-user user-id)})
5 changes: 3 additions & 2 deletions src/clj/villagebook/routes.clj
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@
[villagebook.config :as config]))

(def apiroutes
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(def apiroutes
(def api-routes

{"organisations" {["/" :id] {:get (with-auth org/retrieve)}
:post (with-auth org/create!)}
{"organisations" {:get (with-auth org/retrieve-by-user)
:post (with-auth org/create!)
["/" :id] {:get (with-auth org/retrieve)}}
"user" {:get (with-auth user/retrieve)}})


Expand Down
11 changes: 9 additions & 2 deletions src/cljs/villagebookUI/api/organisation.cljs
Original file line number Diff line number Diff line change
@@ -1,11 +1,18 @@
(ns villagebookUI.api.organisation
Copy link
Member

Choose a reason for hiding this comment

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

Capitals in the ns name seems odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes🤷‍♂ but decided to stick with it, changing would be too much effort.

[:require [ajax.core :refer [POST]]])
[:require [ajax.core :refer [GET POST]]])

(def ^:private create-org-api "/api/organisations")
(def ^:private get-all-orgs-api "/api/organisations")

(defn create-org [org-data handler-fn error-handler-fn]
(defn create! [org-data handler-fn error-handler-fn]
(POST create-org-api
{:params org-data
:format :raw
:handler handler-fn
:error-handler error-handler-fn}))

(defn get-all [handler-fn error-handler-fn]
(GET get-all-orgs-api
{:handler handler-fn
:response-format :json
:error-handler error-handler-fn}))
11 changes: 6 additions & 5 deletions src/cljs/villagebookUI/api/user.cljs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
(ns villagebookUI.api.user
[:require [ajax.core :refer [GET]]])
(:require [ajax.core :refer [GET]]
[villagebookUI.store.user :as store]))

(def ^:private get-user-data-api "/api/user")

(defn get-user-data
[handler-fn error-handler-fn]
(defn get-data [handler-fn error-handler-fn finally-fn]
(GET get-user-data-api
{:handler handler-fn
:error-handler error-handler-fn}))
{:handler handler-fn
:error-handler error-handler-fn
:finally finally-fn}))
Copy link
Member

Choose a reason for hiding this comment

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

Indentation seems off here.

Copy link
Member

Choose a reason for hiding this comment

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

I'd also just call them success-handler, error-handler, and finalizer. The -fn seems unnecessary, but ymmv.

14 changes: 9 additions & 5 deletions src/cljs/villagebookUI/components/create_org.cljs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
(ns villagebookUI.components.create-org
(:require [reagent.core :as r]
[villagebookUI.helpers :refer [random-color handle-esc]]
[villagebookUI.fetchers :as fetchers]
[villagebookUI.components.utils :as utils]
[villagebookUI.api.organisation :as org]))
[villagebookUI.api.organisation :as org-api]))

(defn- create-org [form success-handler error-handler]
(when (:name form)
(org/create-org form
success-handler
error-handler)))
(org-api/create! form
success-handler
error-handler)))

(defn org-creation-form [on-close-handler]
(let [color (random-color)
Expand All @@ -19,7 +20,10 @@
[:form.inline-block {:on-submit (fn [e]
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

(.preventDefault e) can stay perhaps, because it is specific to the view.

(.preventDefault e)
(create-org @form
on-close-handler
(do
(on-close-handler)
(fn [res]
(fetchers/fetch-orgs! last)))
#(reset! error true)))}
[:div.inline-block
[utils/color-picker-patch
Expand Down
12 changes: 6 additions & 6 deletions src/cljs/villagebookUI/components/dashboard.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
(:require [villagebookUI.components.sidebar :refer [sidebar]]
[villagebookUI.components.main-content :refer [main-content]]))

(defn dashboard []
(let []
(fn []
[:div
[sidebar]
[main-content]])))
(defn dashboard [on-mount-cb]
(on-mount-cb)
Copy link
Member

Choose a reason for hiding this comment

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

When in the component lifecycle is this called? Not sure that this gives you good enough control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called once per component (when it's rendered the first time). Reagent provides this construct for setup or initialising local state. Works just like componentDidMount. Relevant doc

(fn []
[:div
[sidebar]
[main-content]]))
6 changes: 4 additions & 2 deletions src/cljs/villagebookUI/components/login.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
(:require [reagent.core :as r]
[accountant.core :as accountant]

[villagebookUI.fetchers :as fetchers]
[villagebookUI.api.auth :as auth]
[villagebookUI.store :as store]))
[villagebookUI.store.user :as user-store]))

(defn validate!
[elementID formdata]
Expand All @@ -27,7 +28,7 @@
(let [formdata (r/atom {})
error (r/atom {})]
(fn []
(if @store/user
(if (user-store/read)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've refactored this here in a separate WIP PR.

(do
(accountant/navigate! "/dashboard")
[:div])
Expand All @@ -46,6 +47,7 @@
(:user @formdata)
(fn [res]
(swap! error assoc :message "")
(fetchers/fetch-user!)
Copy link
Member

Choose a reason for hiding this comment

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

Extract out the on-click function here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will refactor this bit in #14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merged #14 into this branch.

(accountant/navigate! "/dashboard"))
(fn [res]
(swap! error assoc :message (:response res)))))}
Expand Down
6 changes: 4 additions & 2 deletions src/cljs/villagebookUI/components/main_content.cljs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
(ns villagebookUI.components.main-content)
(ns villagebookUI.components.main-content
(:require [villagebookUI.store.organisations :as org-store]))

(defn main-content []
[:div.main-content
[:div.navbar]])
[:div.navbar
[:h5 (:name (org-store/get-selected))]]])
17 changes: 11 additions & 6 deletions src/cljs/villagebookUI/components/sidebar.cljs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
(:require [reagent.core :as r]
[villagebookUI.components.utils :as utils]
[villagebookUI.components.create-org :refer [org-creation-form]]
[villagebookUI.api.organisation :as org]))
[villagebookUI.store.organisations :as org-store]))

(defn sidebar []
(let [creating-org (r/atom false)]
Expand All @@ -13,12 +13,17 @@
[:p.sidebar-logo "villagebook"]
[:div.divider]]
[:div.sidebar-section
[:p.sidebar-section-head "Organisations"]
[:p.sidebar-section-head "Your organisations"]
[:ul.sidebar-section-list
[:li.item
[:a.sidebar-link [utils/patch "#5fcc5f"] "Org 1"]]
[:li.item
[:a.sidebar-link [utils/patch "#ff8383"] "Org 2"]]
(doall
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure the doall is necessary here?

Copy link
Contributor Author

@prabhanshuguptagit prabhanshuguptagit Sep 20, 2019

Choose a reason for hiding this comment

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

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

(for [org (org-store/get-all)
:let [id (:id org)]]
[:li.item {:key (:id org)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[:li.item {:key (:id org)
[:li.item {:key id

:class (if (= id (:id (org-store/get-selected)))
[:active])}
[:a.sidebar-link {:href (str "/orgs/" id)}
[utils/patch (:color org)]
(:name org)]]))
[:li.item
(if @creating-org
[org-creation-form #(reset! creating-org false)]
Expand Down
8 changes: 5 additions & 3 deletions src/cljs/villagebookUI/components/signup.cljs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
(ns villagebookUI.components.signup
(:require [reagent.core :as r]
[reagent.session :as session]
[accountant.core :as accountant]

[villagebookUI.fetchers :as fetchers]
[villagebookUI.api.auth :as auth]
[villagebookUI.store :as store]))
[villagebookUI.api.user :as user-api]
[villagebookUI.store.user :as user-store]))

(defn validate!
[elementID formdata]
Expand All @@ -28,7 +29,7 @@
(let [formdata (r/atom {})
error (r/atom {})]
(fn []
(if @store/user
(if (user-store/read)
(do
(accountant/navigate! "/dashboard")
[:div])
Expand All @@ -49,6 +50,7 @@
(:user @formdata)
(fn [res]
(swap! error assoc :message "")
(fetchers/fetch-user!)
(accountant/navigate! "/dashboard"))
(fn [res]
(swap! error assoc :message (:response res)))))}
Expand Down
14 changes: 8 additions & 6 deletions src/cljs/villagebookUI/core.cljs
Original file line number Diff line number Diff line change
@@ -1,30 +1,32 @@
(ns villagebookUI.core
(:require [reagent.core :as r]
[reagent.session :as session]
[accountant.core :as accountant]
[bidi.bidi :as bidi]

[villagebookUI.fetchers :as fetchers]
[villagebookUI.routes :refer [routes]]
[villagebookUI.store :as store]))
[villagebookUI.store.core :as store]
[villagebookUI.store.session :as session]))

(accountant/configure-navigation!
{:nav-handler (fn [path]
(let [matched-route (bidi/match-route routes path)
current-page (:handler matched-route)
route-params (:route-params matched-route)]
(session/put! :route {:current-page current-page
:route-params route-params})))
(session/set! {:current-page current-page
:route-params route-params})))
:path-exists? (fn [path]
(boolean (bidi/match-route routes path)))})

(defn root []
"Root component holding all other components"
(let [component (:current-page (session/get :route))]
(let [component (session/current-page)]
[component]))

(defn main! []
(store/init!)
(fetchers/fetch-user!)
(accountant/dispatch-current!)
(store/init)
(r/render [root]
(.getElementById js/document "villagebook-app")))

Expand Down
24 changes: 24 additions & 0 deletions src/cljs/villagebookUI/fetchers.cljs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
(ns villagebookUI.fetchers
(:require [villagebookUI.store.user :as user-store]
[villagebookUI.api.user :as user-api]
[villagebookUI.store.organisations :as org-store]
[villagebookUI.api.organisation :as org-api]))


(defn fetch-user! []
(user-api/get-data
(fn [res]
(user-store/add! res))
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this just user-store/add!? Why wrap another fn around it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

(fn [res]
(user-store/add! nil))
#(user-store/fetched!)))

(defn fetch-orgs!
[& [selector-fn]]
(org-api/get-all
(fn [res]
(org-store/add-all! res)
(when selector-fn
(org-store/set-selected!
(selector-fn (org-store/get-all)))))
identity))
8 changes: 7 additions & 1 deletion src/cljs/villagebookUI/helpers.cljs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
(ns villagebookUI.helpers)
(ns villagebookUI.helpers
(:require [villagebookUI.store.session :as session]))

(defn random-color []
(str "hsl(" (->> (.random js/Math)
Expand All @@ -9,3 +10,8 @@
[el handler]
(if (= (.-key el) "Escape")
(handler)))

(defn org-id-from-url []
(-> (session/route-params)
:org-id
js/parseInt))
10 changes: 5 additions & 5 deletions src/cljs/villagebookUI/middleware.cljs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
(ns villagebookUI.middleware
(:require [villagebookUI.store :as store]
(:require [villagebookUI.store.user :as user-store]
[villagebookUI.components.utils :as utils]
[villagebookUI.components.login :refer [login]]))

(defn require-login
[component]
[component & props]
(fn []
(if (store/fetched?)
(if @store/user
[component]
(if (user-store/fetched?)
(if (user-store/read)
[(apply component props)]
[login])
[utils/loading])))
Loading