Skip to content
This repository has been archived by the owner on Apr 24, 2023. It is now read-only.

Create/Edit Article view #24

Merged
merged 1 commit into from
Dec 24, 2017
Merged

Create/Edit Article view #24

merged 1 commit into from
Dec 24, 2017

Conversation

foopang
Copy link
Contributor

@foopang foopang commented Sep 3, 2017

No description provided.

@@ -21,18 +21,39 @@
(cstr/join ", " v)
v))])])

(rum/defc InputFieldContainer [value & children]
(apply vector :fieldset.form-group children))
Copy link
Owner

Choose a reason for hiding this comment

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

do not generate Hiccup forms programatically

(defn TagInputFieldContainer [data errors key]
(fn [value & children]
(let [{:keys [tagList]} @data]
(apply vector :fieldset.form-group children
Copy link
Owner

Choose a reason for hiding this comment

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

do not generate Hiccup forms programatically

:on-blur on-blur
:on-focus on-focus}
events)]
(input-container
Copy link
Owner

Choose a reason for hiding this comment

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

if you need different container elements this most likely should be two separate components instead

:tag {:placeholder "Enter tags"
:container TagInputFieldContainer
:events {:on-key-down
(fn [data errors key]
Copy link
Owner

Choose a reason for hiding this comment

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

not sure what this code is doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is trying to modify the key down behaviour:

  1. to allow the creation of tags by pressing enter, then clear the input field.

  2. Tags will be stored in a virtual form property named tagList which is used for actually sending over to the server.

@@ -22,3 +22,29 @@

(defmethod control :update [_ [id transform data] state]
{:state (merge state (transform data))})

(defmethod control :save [_ [{:keys [title description body tagList]} token id] state]
(let [http-params (if id
Copy link
Owner

Choose a reason for hiding this comment

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

it would be better to make conditional assignments inside of the :http map directly, without creating intermediate values and merging them together

data (atom data-init)
errors (atom errors-init)
fields-init (->> fields
Copy link
Owner

Choose a reason for hiding this comment

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

This code looks complex. Also I think it should be gone since :container and :events config fields is a bit overgeneralization for a single component like InputField.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since form data and form errors are not exposed outside of the form mixin for altering, there is no way to set a new form property for tags. So I come up with this hacky solution by exposing form data form errors atoms to :container and :events for altering, which should make very minimal impact on the form state while still keeping it general enough for other situations. I know it doesn't sound like a good idea, but I think it serves the purpose for now.

@roman01la roman01la merged commit 8529460 into roman01la:master Dec 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants