Adding ui-checkbox element. #31
Conversation
| classes (cond-> (str user-classes " c-checkbox ") | ||
| (contains? legal-styles style) (str (name style)) | ||
| ) | ||
| attrs (cond-> attrs |
There was a problem hiding this comment.
Drop the cond. If everything should be done, then it isn't conditional :)
Also assoc and dissoc can do more than one at a time:
(-> attrs
(assoc :type "checkbox" :className classes :id (name id))
(dissoc :styles))
| user-classes (get attrs :className "") | ||
| classes (cond-> (str user-classes " c-checkbox ") | ||
| (contains? legal-styles style) (str (name style)) | ||
| ) |
There was a problem hiding this comment.
Style guide: no closing brace of any kind should start the non-whitespace of a line. Join this up to the line above.
| ) | ||
| attrs (cond-> attrs | ||
| :always (assoc :type "checkbox") | ||
| :always (dissoc :styles) |
There was a problem hiding this comment.
Type: I think you mean style...but I would not use that, since it has an HTML meaning. See above comments on API of this method.
| :always (assoc :id (name id))) ] | ||
| (dom/span #js {} (dom/input (clj->js attrs)) | ||
| (dom/label #js {:htmlFor id} \u00A0)) | ||
| )) |
|
|
||
| className (optional): additional class stylings to apply to the top level of the checkbox | ||
| id: Name of the checkbox | ||
| style (optional): is-indeterminate :c-checkbox--informative" |
There was a problem hiding this comment.
What you want is:
:checked - Can be one of true, false, or :indeterminate
that way it is compatible with normal checkbox use (true/false), but can support the additional case.
:style should never be used in our elements, since it is an HTML predefined attribute.
You can use (identical? true v) to check that something is the actual boolean value true (or false for that matter).
| (dom/div nil | ||
| (e/ui-checkbox {:id "checkbox"} ) | ||
| (e/ui-checkbox {:id "checkbox" :style #{:is-indeterminate}} ) | ||
| (e/ui-checkbox {:id "checkbox" :style #{:c-checkbox--informative}} ) |
There was a problem hiding this comment.
API should never talk about CSS classes (you're mixing a CSS stye name into a user-visible API). It should be abstract.
| id: Name of the checkbox | ||
| style (optional): is-indeterminate :c-checkbox--informative" | ||
| [{:keys [id style] :or {id ""} :as attrs}] | ||
| (let [legal-styles #{:is-indeterminate :c-checkbox--informative} |
There was a problem hiding this comment.
Informative has been removed as it's an internal-only class we don't need in this project.
|
Merged manually. Still needs fixes, but cannot continue waiting for requested changes. |
No description provided.