-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create categories ui #17
base: api-create-category
Are you sure you want to change the base?
Conversation
d1e8bb6
to
3af43a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] Left a few comments. Looks good otherwise.
|
||
(defn retrieve-by-org | ||
[org-id] | ||
(jdbc/find-by-keys (config/db-spec) :categories {:org_id org-id})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] Use an identifier and hyphenize the records you retrieve.
(let [org-id (edn/read-string (get-in request [:params :org-id])) | ||
user-id (get-in request [:identity :id])] | ||
(if (s/valid? ::org-spec/id org-id) | ||
(utils/model-to-http (models/retrieve-by-org org-id user-id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding the message to the map you pass to utils/model-to-http
with the response codes.
{:message (models/retrieve-by-org org-id user-id) :success ... :error ...}
|
||
(defn create [category org-id handler error-handler] | ||
(POST (create-category-api org-id) | ||
{:body (.stringify js/JSON (clj->js category)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using cheshire
[villagebookUI.fetchers :as fetchers] | ||
[villagebookUI.store.categories :as category-store])) | ||
|
||
(defn- category-active? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] Rename this to category-selected?
?
(on-mount-cb) | ||
(fn [org categories] | ||
[:div.content-box | ||
[:div.category-tabs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] Extract category-tabs out into a different function maybe?
[villagebookUI.api.category :as api] | ||
[villagebookUI.store.organisations :as org-store])) | ||
|
||
(defn submit-create-category |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] This can be renamed to create-category
.
(api/create {:name name | ||
:fields (map #(second %) fields)} | ||
org-id | ||
#(do | ||
(fetchers/fetch-categories! org-id last) | ||
(helpers/show-alert-bottom! :success (str "Category " name " created"))) | ||
#(helpers/show-alert-bottom! :error (:response %))))) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[murtaza] Consider adding all of these parameters to a single map, and destructure it in the function.
(The category properties are added but not being displayed on the frontend atm.)
--
Raised against #12 for reviews only. Change base to master before merge.